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';
@@ -8,6 +8,7 @@ import type { Subject } from '@prisma/client';
 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';
 
@@ -32,7 +33,8 @@ describe('SubjectsService', () => {
             $transaction: vi.fn(),
             instrumentRecord: {
               deleteMany: vi.fn(),
-              findMany: vi.fn()
+              findMany: vi.fn(),
+              groupBy: vi.fn()
             },
             session: {
               deleteMany: vi.fn()
@@ -135,16 +137,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({
@@ -153,18 +148,65 @@ 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',
+        firstName: null,
+        groups: [],
+        id: 'user-1',
+        kind: 'login',
+        lastName: null,
+        mustResetPassword: false,
+        username: 'standard-user'
+      });
+      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';
 
@@ -146,7 +147,7 @@ export class SubjectsService {
           AND: [
             accessibleQuery(ability, 'read', 'Subject'),
             groupInput,
-            { id: { in: await this.querySubjectIdsWithRecords(groupId) } }
+            { id: { in: await this.querySubjectIdsWithRecords(groupId, ability) } }
           ]
         }
       });
@@ -190,12 +191,36 @@ export class SubjectsService {
     return subjects.map((subject) => subject.id);
   }
 
-  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 an `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

@@ -60,7 +60,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>
@@ -170,6 +174,7 @@ const Filters: React.FC<{
           </DropdownMenu.Label>
           <DropdownMenu.CheckboxItem
             checked={hasRecords}
+            data-testid="datahub-filter-has-records"
             onCheckedChange={setHasRecords}
             onSelect={(e) => e.preventDefault()}
           >

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

@@ -4,13 +4,17 @@ import { AppPage } from '../route.page';
 
 export class DatahubPage extends AppPage {
   readonly exportDropdown: Locator;
+  readonly filtersTrigger: Locator;
+  readonly hasRecordsFilter: Locator;
   readonly pageHeader: Locator;
   readonly rowActionsTrigger: Locator;
   readonly rows: Locator;
   readonly searchInput: Locator;
   constructor(page: Page) {
     super(page);
     this.exportDropdown = page.getByTestId('datahub-export-dropdown');
+    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');
@@ -28,4 +32,10 @@ export class DatahubPage extends AppPage {
     await this.$ref.getByRole('menuitem', { exact: true, name: format }).click();
     return started;
   }
+
+  /** 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

@@ -180,4 +180,50 @@ test.describe('data hub', () => {
     expect(payload.length).toBeGreaterThan(0);
     expect([...new Set(payload.map((row) => row.subjectId))]).toStrictEqual([listed]);
   });
+
+  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

@@ -64,7 +64,10 @@ export class ApiClient {
     return group;
   }
 
-  /** Creates a session, and with it the subject it names. */
+  /**
+   * 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>(