diff --git a/.claude/rules/infra/smalruby-classroom.md b/.claude/rules/infra/smalruby-classroom.md index 313907afbfd..4c597a6bc28 100644 --- a/.claude/rules/infra/smalruby-classroom.md +++ b/.claude/rules/infra/smalruby-classroom.md @@ -27,14 +27,15 @@ CDK project for the Classroom service (API Gateway + Lambda + DynamoDB + S3). `X-Google-Access-Token` を含む）。stage スロットリングに加え、`/classrooms/join` と `/classrooms/lookup` はルート個別に厳しめのスロットリングを設定 - **Lambda (Node.js 22)**: 単一ハンドラー (`lambda/handler.ts`) に全ビジネスロジック -- **DynamoDB (8 テーブル)**: - - `Classrooms{suffix}` — GSI `joinCode-index` / `teacherSub-index` +- **DynamoDB (9 テーブル)**: + - `Classrooms{suffix}` — GSI `joinCode-index` / `teacherSub-index` / `groupId-index`（クラスに属する課題。#1146 で `groupId IN (...)` の Scan を置換） - `ClassroomMemberships{suffix}` — GSI `sessionToken-index` - `ClassroomSubmissions{suffix}` — GSI `classroomId-memberId-index` - `ClassroomKickRequests{suffix}` — GSI `classroomId-seatNumber-index`（退室依頼 #692） - `ClassroomGroups{suffix}` — GSI `teacherSub-index`（クラス=学級、長期 TTL 400日） - `SharedAssignments{suffix}` — GSI `status-createdAt-index` / `authorSub-createdAt-index` / `passcode-index`（みんなの課題 #1066。**TTL なし・prod RETAIN + PITR**） - `SharedAssignmentReports{suffix}` — 通報（TTL 90日） + - `ClassroomCoTeacherIndex{suffix}` — 共同管理者の逆引き索引 #1146（PK `coTeacherEmail` / SK `resourceKey` = `assignment#` \| `group#`）。`coTeacherEmails` はリスト属性で GSI にできないため別テーブル。**認可はこの索引を見ない**（真実は item 上のリスト）。既存データは `bin/backfill-coteacher-index.ts` で流し込む - `ClassroomNotifications{suffix}` — お知らせ #1111（PK teacherSub / SK notificationId。**書き手は admin スタックのみ**・この Lambda は Query/UpdateItem だけ grant） - 上記注記のないものは TTL `ttl`・**RemovalPolicy: DESTROY** - **S3** `smalruby-classroom-submissions{suffix}`: 提出ファイル (project.sb3, thumbnail.png, diff --git a/docs/classroom/architecture.md b/docs/classroom/architecture.md index 354d0d7ee11..f7902ada84e 100644 --- a/docs/classroom/architecture.md +++ b/docs/classroom/architecture.md @@ -286,10 +286,15 @@ erDiagram - 先生トークン検証 (`verifyTeacherIdToken`) は `{sub, email}` を返す。Google は `email_verified` のときのみ email を採用、Microsoft は `email` / `preferred_username`。 - 所有権判定は `canManageClassroom(classroom, identity)` = 「`teacherSub === sub` または `coTeacherEmails` に自分の email が含まれる」。全ての先生向け操作で使用。co-teacher は owner と**完全同等**（課題削除・共同管理者の追加/解除も可）。 - 作成者は `teacherSub` で管理され `coTeacherEmails` には含めないため、co-teacher API から作成者を外すことはできない（管理者ゼロを防止）。 -- `GET /classrooms` は次の 3 つの和集合: owner 分（`teacherSub-index`）、課題単位の co-taught 分（課題の `coTeacherEmails` への Scan + `contains` フィルタ）、**管理できるクラス（所有 + 共同管理）に属する課題**（クラスを列挙し、その `groupId` で Classrooms を Scan）。3 つ目は作成者を問わず拾うので、共同管理者にはオーナーが作った課題が、オーナーには共同管理者が作った課題が見える。DynamoDB はリスト属性を GSI 化できないため Scan を使用。Classrooms テーブルは小規模（単一組織・90日 TTL）のため許容。各課題は `role`（owner / co-teacher）を返し、フロントの「共同管理」バッジに使う。 +- `GET /classrooms` は次の 3 つの和集合: owner 分（`teacherSub-index`）、課題単位の co-taught 分（**逆引き索引** `ClassroomCoTeacherIndex` → `BatchGetItem`）、**管理できるクラス（所有 + 共同管理）に属する課題**（管理クラスを列挙し、クラスごとに `groupId-index` を Query）。3 つ目は作成者を問わず拾うので、共同管理者にはオーナーが作った課題が、オーナーには共同管理者が作った課題が見える。各課題は `role`（owner / co-teacher）を返し、フロントの「共同管理」バッジに使う。 +- **一覧経路は Scan を使わない**: Scan は RCU も 1MB 上限も**フィルタ適用「前」**に効くため、コストと取りこぼしのリスクが「その先生に見える量」ではなく**テーブル全体の量**（＝他の先生のデータ量）に比例して増える。そのため課題単位の共同管理は逆引き索引、クラス経由の課題は `groupId-index` GSI に置き換えてある。回帰は `lambda/tests/handler-list-classrooms-reads.test.ts` が「Classrooms / ClassroomGroups への Scan が 0 回」で固定する。 +- **ページング**: DynamoDB の 1MB 上限は（Query でもフィルタ適用「前」に）効くため、`LastEvaluatedKey` を辿らないとデータ量が増えたときにエラー無しで取りこぼす。一覧系の Query は共通ヘルパー `queryAll` で最後のページまで辿り、キー指定でまとめて読む経路は `batchGetAll`（100 キーずつ + `UnprocessedKeys` の再試行 — スロットリングは**エラーではなく未処理キーの返却**で表れるので、捨てると同じ「黙って取りこぼす」形になる）で読む。対象は `GET /classrooms` / `GET /classroom-groups` の読み取りに加え、**列挙結果がそのまま書き込みを駆動する経路**（組への移行 `POST /classroom-groups/migrate`、クラス人数の課題への伝播、課題名の重複連番、トピックのカスケード）も含む — ここで取りこぼすと「一部だけ更新されない」状態になり、読み取りの取りこぼしより気付きにくい。無限ループ防止に `DDB_MAX_PAGES`（既定 25、不正値は既定に戻す）でページ数を打ち切り、打ち切り時は CloudWatch に warn を残す。単一クラス内に閉じた Query（メンバー・提出物・退室依頼）と `Limit` 付きの「上位 N 件」系はページングしない。 - **クラス（組）単位の共同管理**の判定は `canManageGroup(group, identity)` = 「`teacherSub === sub` または クラスの `coTeacherEmails` に自分の email が含まれる」。クラス単位の共同管理者は、そのクラスの中では owner と同等に振る舞える（課題の一覧・作成・更新・複製・トピック管理・クラス設定の編集）。**例外はクラスの共同管理者リストそのもの**（`PATCH /classroom-groups/{groupId}` の `coTeacherEmails`）で、これは owner のみ変更できる（共同管理者が勝手に招待・自己解除できないようにするため）。 - 共同管理者の email は**書き込み時**に正規化（`validateCoTeacherEmail` = trim + 小文字）して保存する。認可判定（`canManageClassroom` / `canManageGroup`）は両辺を正規化して比較するので大文字小文字に左右されない。ただし一覧の絞り込みに使う DynamoDB の `contains` は完全一致（大文字小文字を区別）なので、**一覧に出るかどうかは保存側が正規化されている前提**に依存する（手で書き換えた項目に注意）。 - 所有者でも共同管理者でもない先生には、クラスの存在を秘匿して **404 `Group not found`** を返す。 +- **`ClassroomCoTeacherIndex` テーブル（逆引き索引）**: PK `coTeacherEmail` / SK `resourceKey`（`assignment#` または `group#`）。`coTeacherEmails` はリスト属性なので GSI のキーにできず、逆引きは別テーブルになる。行は `syncCoTeacherIndex()` が**差分だけ**書き（課題の共同管理者 追加/削除、組の `coTeacherEmails` 更新、v2 移行での組への引き上げ — この 4 経路すべてが書き手）、資源の `ttl` を写すので資源と一緒に消える。 + - **認可は索引を見ない**。`canManageClassroom` / `canManageGroup` は今も item 上の `coTeacherEmails` を読む。索引は「一覧に出す候補を集める」ためだけのもので、読み取り側は引いた資源のリストで再確認する（索引が遅れても権限は落ちず、一覧に出るのが遅れるだけ）。 + - **既存データにはバックフィルが必要**（索引は後付けなので、それ以前の共同管理者の行が存在しない）。`bin/backfill-coteacher-index.ts`（既定 dry-run・冪等）を**読み取りを索引に切り替えた Lambda を反映する前に**実行する。デプロイ順と手順は `docs/classroom/operations.md`。 ### ClassroomMemberships テーブル diff --git a/docs/classroom/operations.md b/docs/classroom/operations.md index d5b2f383331..d49a0799977 100644 --- a/docs/classroom/operations.md +++ b/docs/classroom/operations.md @@ -74,6 +74,57 @@ npx ts-node bin/restore-classroom.ts --classroom-id --apply - 提出ファイル（S3 実体）の存在確認を行い、欠落があれば dry-run 時に警告表示 - 課題本体の書き込みは `attribute_not_exists` 条件付き（復元の競合で生きているデータを上書きしない） +## 共同管理者の逆引き索引バックフィル `bin/backfill-coteacher-index.ts` + +`GET /classrooms` は共同管理の課題・クラスを **逆引き索引** `ClassroomCoTeacherIndex{suffix}` +（PK `coTeacherEmail` / SK `resourceKey`）から集める。索引は後付けなので、**それ以前に登録された +共同管理者の行は存在しない** — 流し込まないまま読み取りを索引に切り替えると、既存の共同管理者から +課題・クラスが**一覧に出なくなる**（権限そのものは item 上の `coTeacherEmails` で判定しているので +落ちない。直リンクなら操作できてしまうぶん気付きにくい）。 + +**デプロイ順（守る）:** + +1. `npx cdk deploy` — 索引テーブルと `groupId-index` GSI を作る +2. **`groupId-index` が `ACTIVE` になるまで待つ**（下記） +3. **バックフィルを `--apply` で実行**（下記） +4. 読み取りを索引に切り替えた Lambda を反映する `npx cdk deploy` + +1 と 4 が同じ deploy に乗る場合（=通常のリリース）は、**deploy 直後に速やかに 2→3 を実行する**。 +その間だけ共同管理の資源が一覧に出ない。 + +**GSI の待ちを飛ばさないこと。** GSI は作成直後 `CREATING`（`Backfilling: true`）で、この間は +その索引への Query が失敗しうる。**項目が 100 件程度の stg でも ACTIVE まで約 5 分かかった**実績が +あるので、本番ではもう少し見ておく。 + +```bash +# ACTIVE になるまで待つ +until [ "$(aws dynamodb describe-table --table-name Classrooms \ + --query 'Table.GlobalSecondaryIndexes[?IndexName==`groupId-index`].IndexStatus' --output text)" = ACTIVE ]; do + sleep 20 +done +``` + +```bash +# デフォルトは dry-run（何件・どの行を作るかだけ表示） +npx ts-node bin/backfill-coteacher-index.ts + +# 確認後に実行 +npx ts-node bin/backfill-coteacher-index.ts --apply +``` + +**dry-run は索引テーブルが無くても動く**（既存テーブルを読むだけ）。デプロイ前に流して +「何行できるはずか」を先に確認しておくと、`--apply` の結果と突き合わせられるうえ、 +一覧に出ない窓を短くできる。ステージは `.env` の symlink で切り替える +（`ln -sfn .env.prod .env`。`.claude/rules/infra/development.md`）。 + +- **冪等**。行のキーは `(coTeacherEmail, resourceKey)` なので、何度実行しても同じ行を上書きするだけ。 + 取りこぼしが疑われるときは再実行して差分を埋めてよい +- 対象は Classrooms / ClassroomGroups の**全件**（アーカイブ済みも含む。`?includeArchived=1` の + 一覧からも共同管理者に見えるべきで、消えるのは TTL に任せる） +- 索引行は資源の `ttl` を写すので、資源と一緒に自然消滅する +- email は保存時と同じ正規化（trim + 小文字）でキーにする。**手で書き換えた項目**が正規化されて + いないと索引に載らない（一覧に出ない）ので、その場合は項目側を直してから再実行する + ## よくあるケース | 状況 | 対応 | diff --git a/infra/smalruby-admin/lambda/tests/handler-classroom-groups.test.ts b/infra/smalruby-admin/lambda/tests/handler-classroom-groups.test.ts index fae6f83d9e6..30747be7de1 100644 --- a/infra/smalruby-admin/lambda/tests/handler-classroom-groups.test.ts +++ b/infra/smalruby-admin/lambda/tests/handler-classroom-groups.test.ts @@ -8,6 +8,10 @@ * 固定する（いずれも壊れると運用事故になる不変条件）。 */ +// このファイルをモジュールにするための宣言。import/export を持たない .ts は TypeScript の +// グローバルスクリプト扱いになり、トップレベルの const がテストファイル間で衝突する（#1144）。 +export {}; + const mockSend = jest.fn(); jest.mock('@aws-sdk/lib-dynamodb', () => { const actual = jest.requireActual('@aws-sdk/lib-dynamodb'); diff --git a/infra/smalruby-classroom/bin/backfill-coteacher-index.ts b/infra/smalruby-classroom/bin/backfill-coteacher-index.ts new file mode 100644 index 00000000000..05d20ce6a45 --- /dev/null +++ b/infra/smalruby-classroom/bin/backfill-coteacher-index.ts @@ -0,0 +1,122 @@ +#!/usr/bin/env npx ts-node +/** + * Operator CLI: 既存データから共同管理者の逆引き索引 + * (`ClassroomCoTeacherIndex{suffix}`) を作り直す一回限りのバックフィル + * (issue #1146)。 + * + * `GET /classrooms` が索引を読むようになったので、**索引に行が無い共同管理者 + * には資源が見えない**。索引は #1146 で追加したため、それ以前に登録された + * 共同管理者の行は存在しない。したがってデプロイ順は: + * + * 1. テーブル + GSI を作る（このスクリプトの前提。cdk deploy） + * 2. **このスクリプトを --apply で実行** + * 3. 読み取りを索引に切り替えた Lambda を反映（cdk deploy） + * + * 1 と 3 が同じ cdk deploy に乗る場合は、deploy 直後に速やかに 2 を実行する + * （その間は共同管理の資源が一覧に出ない。認可は item 上の coTeacherEmails を + * 見ているので、権限そのものは落ちない）。 + * + * 冪等: 索引行のキーは (email, resourceKey) なので、何度実行しても同じ行を + * 上書きするだけ。夜間などに再実行して差分を埋めても安全。 + * + * Usage (from infra/smalruby-classroom, with AWS credentials for the stage): + * npx ts-node bin/backfill-coteacher-index.ts # dry-run（既定） + * npx ts-node bin/backfill-coteacher-index.ts --apply # 実際に書く + * + * The stage comes from the .env symlink (STAGE), same as cdk deploy. + * Full runbook: docs/classroom/operations.md + */ +import 'dotenv/config'; +import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; +import { DynamoDBDocumentClient, BatchWriteCommand, ScanCommand } from '@aws-sdk/lib-dynamodb'; +import { resourceNamesForStage } from '../lambda/restore-lib'; +import { + chunkIndexWrites, + indexRowsForResource, + parseBackfillArgs, + type CoTeacherIndexRow, +} from '../lambda/coteacher-index-lib'; + +const stage = process.env.STAGE || 'stg'; +const names = resourceNamesForStage(stage); +const docClient = DynamoDBDocumentClient.from(new DynamoDBClient({})); + +/** + * テーブル全件を読む。バックフィルは「全部読む」以外に手が無いので、ここは + * 意図的に Scan（一回限りの運用作業であり、リクエスト経路ではない）。 + * @param tableName - 読むテーブル + */ +async function scanEverything(tableName: string): Promise[]> { + const items: Record[] = []; + let startKey: Record | undefined; + do { + const res = await docClient.send(new ScanCommand({ + TableName: tableName, + ExclusiveStartKey: startKey, + })); + items.push(...((res.Items || []) as Record[])); + startKey = res.LastEvaluatedKey as Record | undefined; + } while (startKey); + return items; +} + +async function writeRows(rows: CoTeacherIndexRow[]): Promise { + let written = 0; + for (const chunk of chunkIndexWrites(rows)) { + let pending = chunk.map(Item => ({ PutRequest: { Item } })); + for (let attempt = 0; pending.length > 0 && attempt < 10; attempt++) { + const res = await docClient.send(new BatchWriteCommand({ + RequestItems: { [names.coTeacherIndexTable]: pending }, + })); + written += pending.length; + // DynamoDB はスロットリングをエラーではなく「未処理分の返却」で表すので、 + // ここを捨てると静かに一部だけ書かれた索引になる。 + const unprocessed = (res.UnprocessedItems?.[names.coTeacherIndexTable] || []) as typeof pending; + written -= unprocessed.length; + pending = unprocessed; + } + if (pending.length > 0) { + throw new Error(`failed to write ${pending.length} index rows after retries`); + } + } + return written; +} + +async function main(): Promise { + const { apply } = parseBackfillArgs(process.argv.slice(2)); + console.log(`stage=${stage} index=${names.coTeacherIndexTable} mode=${apply ? 'APPLY' : 'dry-run'}`); + + const [classrooms, groups] = await Promise.all([ + scanEverything(names.classroomsTable), + scanEverything(names.groupsTable), + ]); + + const rows = [ + ...classrooms.flatMap(item => indexRowsForResource('assignment', item)), + ...groups.flatMap(item => indexRowsForResource('group', item)), + ]; + const emails = new Set(rows.map(r => r.coTeacherEmail)); + console.log( + `scanned: ${classrooms.length} assignments / ${groups.length} groups` + + ` → ${rows.length} index rows for ${emails.size} co-teacher emails`, + ); + + if (!apply) { + for (const row of rows.slice(0, 20)) { + console.log(` would put ${row.coTeacherEmail} ${row.resourceKey}`); + } + if (rows.length > 20) { + console.log(` ... and ${rows.length - 20} more`); + } + console.log('dry-run: nothing written. re-run with --apply'); + return; + } + + const written = await writeRows(rows); + console.log(`wrote ${written} index rows`); +} + +main().catch(err => { + console.error(err); + process.exit(1); +}); diff --git a/infra/smalruby-classroom/lambda/coteacher-index-lib.ts b/infra/smalruby-classroom/lambda/coteacher-index-lib.ts new file mode 100644 index 00000000000..c685ecbae14 Binary files /dev/null and b/infra/smalruby-classroom/lambda/coteacher-index-lib.ts differ diff --git a/infra/smalruby-classroom/lambda/handler.ts b/infra/smalruby-classroom/lambda/handler.ts index 87b8916998a..1bac7a69f36 100644 --- a/infra/smalruby-classroom/lambda/handler.ts +++ b/infra/smalruby-classroom/lambda/handler.ts @@ -5,10 +5,10 @@ import { PutCommand, GetCommand, QueryCommand, - ScanCommand, DeleteCommand, UpdateCommand, BatchWriteCommand, + BatchGetCommand, } from '@aws-sdk/lib-dynamodb'; import { S3Client, @@ -30,6 +30,8 @@ const MEMBERSHIPS_TABLE = process.env.MEMBERSHIPS_TABLE_NAME || 'ClassroomMember const SUBMISSIONS_TABLE = process.env.SUBMISSIONS_TABLE_NAME || 'ClassroomSubmissions'; const KICK_REQUESTS_TABLE = process.env.KICK_REQUESTS_TABLE_NAME || 'ClassroomKickRequests'; const GROUPS_TABLE = process.env.GROUPS_TABLE_NAME || 'ClassroomGroups'; +// 共同管理者 email → 資源 の逆引き（#1146）。list 属性は GSI にできないため別テーブル。 +const CO_TEACHER_INDEX_TABLE = process.env.CO_TEACHER_INDEX_TABLE_NAME || 'ClassroomCoTeacherIndex'; const SUBMISSIONS_BUCKET = process.env.SUBMISSIONS_BUCKET_NAME || 'smalruby-classroom-submissions'; // みんなの課題 — nationwide shared assignment library (EPIC #1066) const SHARED_ASSIGNMENTS_TABLE = process.env.SHARED_ASSIGNMENTS_TABLE_NAME || 'SharedAssignments'; @@ -152,6 +154,86 @@ const docClient = DynamoDBDocumentClient.from(ddbClient, { marshallOptions: { removeUndefinedValues: true }, }); +// --- Paginated reads (#1146) --- + +// DynamoDB は 1 回の Query で最大 1MB しか読まず、続きは +// LastEvaluatedKey で辿る。しかもこの 1MB 上限は **FilterExpression 適用前** +// に効くため、辿らないとテーブルが育った時点でエラーも出さずに黙って +// 取りこぼす。一覧系の読み取りは下の queryAll に寄せる。 +// 上限は暴走（壊れたページャで無限ループ）を防ぐ保険。 +// 不正値（NaN / 0 以下）はそのまま使うと 1 ページも読まずに空配列を返し、 +// このヘルパーが無くそうとしている「黙って取りこぼす」挙動そのものになる。 +// 既定へフォールバックして、設定ミスが一覧の消失に化けないようにする。 +const parsedMaxPages = parseInt(process.env.DDB_MAX_PAGES || '25', 10); +const MAX_PAGES = Number.isFinite(parsedMaxPages) && parsedMaxPages > 0 ? parsedMaxPages : 25; + +/** + * Query を LastEvaluatedKey が無くなるまで辿って全項目を返す。 + * `Limit` 付き（= 上位 N 件だけ欲しい）の呼び出しには使わない。 + */ +async function paginateAll( + makeCommand: (startKey?: Record) => QueryCommand, +): Promise[]> { + const items: Record[] = []; + let startKey: Record | undefined; + for (let page = 0; page < MAX_PAGES; page++) { + const result = await docClient.send(makeCommand(startKey)); + items.push(...((result.Items || []) as Record[])); + startKey = result.LastEvaluatedKey as Record | undefined; + if (!startKey) { + return items; + } + } + // ページ上限に達した = 想定より遥かに大きいテーブル。取りこぼしは + // 起きるが、無限ループよりは検知可能な形（ログ）で止める。 + console.warn('[ddb] pagination truncated at MAX_PAGES', { maxPages: MAX_PAGES, items: items.length }); + return items; +} + +/** + * ページングする Query（GSI 一覧など）。 + * Scan 版は置かない — 一覧経路の Scan は #1146 で全廃したので、 + * 「ページングしてくれる Scan」を用意しておくと表全体を読む経路が + * 再び生えやすくなる（必要になったらそのとき書く）。 + */ +async function queryAll(input: QueryCommand['input']): Promise[]> { + return paginateAll(startKey => new QueryCommand({ ...input, ExclusiveStartKey: startKey })); +} + +// BatchGetItem は 1 リクエスト 100 キーまで。 +const BATCH_GET_LIMIT = 100; + +/** + * Fetch many items by key, in chunks of 100, retrying whatever DynamoDB hands + * back as `UnprocessedKeys` (it throttles by returning keys, not by erroring — + * dropping them would be the same silent-loss failure mode as an untraversed + * `LastEvaluatedKey`). Keys that no longer exist are simply absent from the + * result, which is what a lagging reverse-index row should look like (#1146). + * @param tableName - table to read from + * @param keys - primary keys to fetch (duplicates are fine) + * @returns the items that still exist, in no particular order + */ +async function batchGetAll( + tableName: string, + keys: Record[], +): Promise[]> { + const items: Record[] = []; + for (let i = 0; i < keys.length; i += BATCH_GET_LIMIT) { + let pending = keys.slice(i, i + BATCH_GET_LIMIT); + for (let attempt = 0; pending.length > 0 && attempt < MAX_PAGES; attempt++) { + const result = await docClient.send(new BatchGetCommand({ + RequestItems: { [tableName]: { Keys: pending } }, + })); + items.push(...((result.Responses?.[tableName] || []) as Record[])); + pending = (result.UnprocessedKeys?.[tableName]?.Keys || []) as Record[]; + } + if (pending.length > 0) { + console.warn('[ddb] batchGet gave up on unprocessed keys', { tableName, pending: pending.length }); + } + } + return items; +} + // --- S3 Client --- const s3Client = new S3Client({}); @@ -556,14 +638,15 @@ async function handleCreateClassroom(identity: TeacherIdentity, body: Record (groupId ? true : item.className === className) && item.status === 'active') .map(item => item.assignmentName as string); @@ -626,26 +709,121 @@ async function handleCreateClassroom(identity: TeacherIdentity, body: Record { + const beforeSet = new Set(before.map(normalizeEmail)); + const afterSet = new Set(after.map(normalizeEmail)); + const resourceKey = coTeacherResourceKey(type, id); + const now = new Date().toISOString(); + const writes: Promise[] = []; + for (const email of afterSet) { + if (!beforeSet.has(email)) { + writes.push(docClient.send(new PutCommand({ + TableName: CO_TEACHER_INDEX_TABLE, + Item: { + coTeacherEmail: email, + resourceKey, + resourceType: type, + resourceId: id, + updatedAt: now, + ...(typeof ttl === 'number' ? { ttl } : {}), + }, + }))); + } + } + for (const email of beforeSet) { + if (!afterSet.has(email)) { + writes.push(docClient.send(new DeleteCommand({ + TableName: CO_TEACHER_INDEX_TABLE, + Key: { coTeacherEmail: email, resourceKey }, + }))); + } + } + await Promise.all(writes); +} + /** Read the coTeacherEmails list from a classroom item, defaulting to []. */ function readCoTeacherEmails(item: Record): string[] { return Array.isArray(item.coTeacherEmails) ? (item.coTeacherEmails as string[]) : []; } /** - * Classes (組) shared with this teacher as a class-level co-teacher. DynamoDB - * cannot index a list attribute, so this is a Scan + filter — the groups table - * is small (one row per class) and this runs on list-style requests only. + * Ids of the resources of one kind shared with this email, read off the + * reverse index instead of scanning the resource table (#1146). The sort key + * is `#`, so one `begins_with` picks either kind. + * @param email - the co-teacher's verified email (already normalized) + * @param type - assignment (課題) or group (組) + * @returns the resource ids, deduplicated + */ +async function listCoTeacherResourceIds( + email: string, + type: CoTeacherResourceType, +): Promise { + const rows = await queryAll({ + TableName: CO_TEACHER_INDEX_TABLE, + KeyConditionExpression: 'coTeacherEmail = :email AND begins_with(resourceKey, :prefix)', + ExpressionAttributeValues: { ':email': email, ':prefix': `${type}#` }, + }); + const ids = new Set(); + for (const row of rows) { + if (typeof row.resourceId === 'string' && row.resourceId) { + ids.add(row.resourceId); + } + } + return [...ids]; +} + +/** + * Classes (組) shared with this teacher as a class-level co-teacher. Read via + * the co-teacher reverse index + BatchGet, so the cost scales with how many + * classes this teacher co-manages rather than with the size of the groups + * table — which matters because `ClassroomGroups` has a 400-day TTL and so + * accumulates (#1146). */ async function listCoManagedGroups(identity: TeacherIdentity): Promise[]> { if (!identity.email) { return []; } - const scanned = await docClient.send(new ScanCommand({ - TableName: GROUPS_TABLE, - FilterExpression: 'contains(coTeacherEmails, :email)', - ExpressionAttributeValues: { ':email': normalizeEmail(identity.email) }, - })); - return (scanned.Items || []).filter(item => item.teacherSub !== identity.sub); + const ids = await listCoTeacherResourceIds(normalizeEmail(identity.email), 'group'); + if (ids.length === 0) { + return []; + } + const groups = await batchGetAll(GROUPS_TABLE, ids.map(groupId => ({ groupId }))); + // The index is eventually consistent with the item, and authorization still + // reads the item's own list, so re-check it here rather than trusting the row. + return groups.filter(item => item.teacherSub !== identity.sub + && emailInCoTeacherList(item.coTeacherEmails, identity.email)); } /** @@ -653,14 +831,14 @@ async function listCoManagedGroups(identity: TeacherIdentity): Promise { - const owned = await docClient.send(new QueryCommand({ + const owned = await queryAll({ TableName: GROUPS_TABLE, IndexName: 'teacherSub-index', KeyConditionExpression: 'teacherSub = :ts', ExpressionAttributeValues: { ':ts': identity.sub }, - })); + }); const ids = new Set(); - for (const group of [...(owned.Items || []), ...(await listCoManagedGroups(identity))]) { + for (const group of [...owned, ...(await listCoManagedGroups(identity))]) { if (typeof group.groupId === 'string') { ids.add(group.groupId); } @@ -669,28 +847,46 @@ async function listManageableGroupIds(identity: TeacherIdentity): Promise[]> { + const ids = await listCoTeacherResourceIds(email, 'assignment'); + if (ids.length === 0) { + return []; + } + const items = await batchGetAll(CLASSROOMS_TABLE, ids.map(classroomId => ({ classroomId }))); + // Re-check against the item itself: the index may lag a removal, and + // authorization has always been decided by the list stored on the item. + return items.filter(item => emailInCoTeacherList(item.coTeacherEmails, email)); +} + +/** + * Assignments filed under any of the given classes, whoever created them. + * Queried through the `groupId-index` GSI, one Query per class, so the cost + * scales with the assignments actually in those classes instead of with the + * whole table (#1146). Managing a class grants access to everything inside it + * regardless of who created each assignment (#1138), so this covers both + * directions and replaces the old `groupId IN (...)` Scan — with it the + * 100-operand `IN` cap, and the chunking it forced, are gone. */ async function listAssignmentsInGroups(groupIds: string[]): Promise[]> { - const items: Record[] = []; - for (let i = 0; i < groupIds.length; i += 100) { - const chunk = groupIds.slice(i, i + 100); - const values: Record = {}; - const placeholders = chunk.map((id, index) => { - values[`:g${index}`] = id; - return `:g${index}`; - }); - const scanned = await docClient.send(new ScanCommand({ + const byId = new Map>(); + for (const groupId of new Set(groupIds)) { + const items = await queryAll({ TableName: CLASSROOMS_TABLE, - FilterExpression: `groupId IN (${placeholders.join(', ')})`, - ExpressionAttributeValues: values, - })); - items.push(...(scanned.Items || [])); + IndexName: 'groupId-index', + KeyConditionExpression: 'groupId = :gid', + ExpressionAttributeValues: { ':gid': groupId }, + }); + for (const item of items) { + byId.set(item.classroomId as string, item); + } } - return items; + return [...byId.values()]; } /** Fetch the owning class (group) of an assignment, or undefined (pre-v2). */ @@ -750,42 +946,39 @@ function mapClassroomSummary(item: Record, identity: TeacherIde } async function handleListClassrooms(identity: TeacherIdentity, includeArchived = false): Promise { - // Classes the teacher owns — fast GSI query on teacherSub. - const owned = await docClient.send(new QueryCommand({ + // Assignments the teacher owns — fast GSI query on teacherSub. + const owned = await queryAll({ TableName: CLASSROOMS_TABLE, IndexName: 'teacherSub-index', KeyConditionExpression: 'teacherSub = :ts', ExpressionAttributeValues: { ':ts': identity.sub }, - })); - - // Classes the teacher co-manages — matched by verified email. DynamoDB cannot - // index a list attribute with a GSI, so we Scan + filter on coTeacherEmails. - // The classrooms table is small (single org, 30-day TTL) so this is fine; - // revisit with a reverse-index table (email -> classroomId) if it grows large. - let coManaged: Record[] = []; - if (identity.email) { - const scan = await docClient.send(new ScanCommand({ - TableName: CLASSROOMS_TABLE, - FilterExpression: 'contains(coTeacherEmails, :email)', - ExpressionAttributeValues: { ':email': normalizeEmail(identity.email) }, - })); - coManaged = scan.Items || []; - } + }); - // Assignments that live inside a class (組) the teacher may manage — the ones - // they own *and* the ones they co-manage. Managing a class grants access to - // everything inside it regardless of who created each assignment, so this - // covers both directions (issue #1138): a co-teacher must see the owner's - // assignments, and the owner must see assignments a co-teacher created. - // Keying off teacherSub would miss the other party, so filter by groupId. - const viaGroup = await listAssignmentsInGroups(await listManageableGroupIds(identity)); + // Everything else the teacher may see. Both halves are index lookups, so + // this request no longer Scans the classrooms or the groups table at all + // (#1146) — the cost now scales with what this teacher can see, not with how + // much data every other teacher has accumulated: + // - assignments co-taught one by one, via the co-teacher reverse index + // (a list attribute cannot be a GSI key, hence the separate table); + // - assignments living inside a class (組) the teacher may manage, whether + // they own that class or co-manage it, via the `groupId-index` GSI. + // Managing a class grants access to everything inside it regardless of + // who created each assignment, so this covers both directions (issue + // #1138): a co-teacher must see the owner's assignments, and the owner + // must see assignments a co-teacher created. Keying off teacherSub would + // miss the other party, so key off groupId. + const [coTaught, inGroups] = await Promise.all([ + identity.email ? listAssignmentsCoTaughtBy(normalizeEmail(identity.email)) : [], + listManageableGroupIds(identity).then(listAssignmentsInGroups), + ]); + const shared = [...coTaught, ...inGroups]; // Merge by classroomId (owned wins). Archived classes are excluded unless // the caller opts in with ?includeArchived=1 (the archive-list / restore UI, // issue #1050) — the default stays active-only so already-deployed frontends // never see archived items reappear. const byId = new Map>(); - for (const item of [...(owned.Items || []), ...coManaged, ...viaGroup]) { + for (const item of [...owned, ...shared]) { const id = item.classroomId as string; const visible = item.status === 'active' || (includeArchived && item.status === 'archived'); if (visible && !byId.has(id)) { @@ -893,6 +1086,7 @@ async function handleAddCoTeacher(identity: TeacherIdentity, classroomId: string UpdateExpression: 'SET coTeacherEmails = :list, updatedAt = :now', ExpressionAttributeValues: { ':list': updated, ':now': new Date().toISOString() }, })); + await syncCoTeacherIndex('assignment', classroomId, existing, updated, result.Item.ttl); return { statusCode: 200, body: JSON.stringify({ coTeacherEmails: updated }) }; } @@ -920,6 +1114,7 @@ async function handleRemoveCoTeacher(identity: TeacherIdentity, classroomId: str UpdateExpression: 'SET coTeacherEmails = :list, updatedAt = :now', ExpressionAttributeValues: { ':list': updated, ':now': new Date().toISOString() }, })); + await syncCoTeacherIndex('assignment', classroomId, existing, updated, result.Item.ttl); } return { statusCode: 200, body: JSON.stringify({ coTeacherEmails: updated }) }; @@ -2529,14 +2724,14 @@ async function handleCreateGroup(identity: TeacherIdentity, body: Record { - const result = await docClient.send(new QueryCommand({ + const result = await queryAll({ TableName: GROUPS_TABLE, IndexName: 'teacherSub-index', KeyConditionExpression: 'teacherSub = :ts', ExpressionAttributeValues: { ':ts': identity.sub }, - })); + }); const coManaged = await listCoManagedGroups(identity); - const groups = [...(result.Items || []), ...coManaged].map(item => ({ + const groups = [...result, ...coManaged].map(item => ({ ...mapGroupSummary(item), role: item.teacherSub === identity.sub ? 'owner' : 'co-teacher', })); @@ -2606,6 +2801,16 @@ async function handleUpdateGroup(identity: TeacherIdentity, groupId: string, bod ReturnValues: 'ALL_NEW', })); + if (updates.coTeacherEmails !== undefined) { + await syncCoTeacherIndex( + 'group', + groupId, + readCoTeacherEmails(group), + updates.coTeacherEmails as string[], + group.ttl, + ); + } + // The class's studentCount is authoritative, so a change must flow down to // its existing assignments (classrooms) — otherwise growing/shrinking the // class leaves old assignments on their creation-time snapshot. Increasing @@ -2655,21 +2860,23 @@ async function propagateStudentCountToClassrooms( * enumerate from, and each teacher migrates their own account (#1145). */ async function handleMigrateGroups(identity: TeacherIdentity): Promise { + // Paginated: the plan drives writes (create classes, adopt assignments), so + // a truncated read would silently leave part of the account unmigrated. const [classroomsResult, groupsResult] = await Promise.all([ - docClient.send(new QueryCommand({ + queryAll({ TableName: CLASSROOMS_TABLE, IndexName: 'teacherSub-index', KeyConditionExpression: 'teacherSub = :ts', ExpressionAttributeValues: { ':ts': identity.sub }, - })), - docClient.send(new QueryCommand({ + }), + queryAll({ TableName: GROUPS_TABLE, IndexName: 'teacherSub-index', KeyConditionExpression: 'teacherSub = :ts', ExpressionAttributeValues: { ':ts': identity.sub }, - })), + }), ]); - const plan = planGroupMigration(classroomsResult.Items || [], groupsResult.Items || []); + const plan = planGroupMigration(classroomsResult, groupsResult); const now = new Date().toISOString(); const groupIdByKey = new Map(); @@ -2728,6 +2935,20 @@ async function handleMigrateGroups(identity: TeacherIdentity): Promise 0 ? { ExpressionAttributeNames: names } : {}), ExpressionAttributeValues: values, })); + // Migration lifts each assignment's co-teachers up onto the class, so this + // is another writer of coTeacherEmails and must keep the reverse index in + // step — otherwise a migrated teacher's shared classes stay invisible to + // the index-based reads above (#1146). + if (update.set.coTeacherEmails !== undefined) { + const existing = groupsResult.find(g => String(g.groupId) === update.groupKey); + await syncCoTeacherIndex( + 'group', + groupId, + existing ? readCoTeacherEmails(existing) : [], + update.set.coTeacherEmails as string[], + existing ? existing.ttl : undefined, + ); + } } return { diff --git a/infra/smalruby-classroom/lambda/restore-lib.ts b/infra/smalruby-classroom/lambda/restore-lib.ts index 56e877f54c0..25ccffe6426 100644 --- a/infra/smalruby-classroom/lambda/restore-lib.ts +++ b/infra/smalruby-classroom/lambda/restore-lib.ts @@ -163,6 +163,7 @@ export function resourceNamesForStage(stage: string): { membershipsTable: string; submissionsTable: string; groupsTable: string; + coTeacherIndexTable: string; bucket: string; } { const suffix = stage === 'prod' ? '' : `-${stage}`; @@ -171,6 +172,7 @@ export function resourceNamesForStage(stage: string): { membershipsTable: `ClassroomMemberships${suffix}`, submissionsTable: `ClassroomSubmissions${suffix}`, groupsTable: `ClassroomGroups${suffix}`, + coTeacherIndexTable: `ClassroomCoTeacherIndex${suffix}`, bucket: `smalruby-classroom-submissions${suffix}`, }; } diff --git a/infra/smalruby-classroom/lambda/tests/coteacher-index-lib.test.ts b/infra/smalruby-classroom/lambda/tests/coteacher-index-lib.test.ts new file mode 100644 index 00000000000..7de976d709a --- /dev/null +++ b/infra/smalruby-classroom/lambda/tests/coteacher-index-lib.test.ts @@ -0,0 +1,109 @@ + +/** + * 逆引き索引バックフィルの純粋関数のテスト (issue #1146)。 + * + * バックフィルを取りこぼすと「既存の共同管理者から資源が見えない」という形で + * 壊れる（権限は item 上の coTeacherEmails で判定するので直リンクでは操作でき、 + * 一覧に出ないだけ = 気付きにくい）。行の作り方をここで固定する。 + */ + +import { + BATCH_WRITE_LIMIT, + chunkIndexWrites, + indexRowsForResource, + normalizeIndexEmail, + parseBackfillArgs, +} from '../coteacher-index-lib'; +import { resourceNamesForStage } from '../restore-lib'; + +describe('indexRowsForResource', () => { + test('課題 1 件から共同管理者ごとの行を作り、ttl を写す', () => { + expect(indexRowsForResource('assignment', { + classroomId: 'c1', + coTeacherEmails: ['A@Example.com', 'b@example.com'], + ttl: 1800000000, + })).toEqual([ + { + coTeacherEmail: 'a@example.com', resourceKey: 'assignment#c1', + resourceType: 'assignment', resourceId: 'c1', ttl: 1800000000, + }, + { + coTeacherEmail: 'b@example.com', resourceKey: 'assignment#c1', + resourceType: 'assignment', resourceId: 'c1', ttl: 1800000000, + }, + ]); + }); + + test('組は group# 接頭辞になる（同じ email で課題と組を撃ち分けるため）', () => { + expect(indexRowsForResource('group', { groupId: 'g1', coTeacherEmails: ['a@example.com'] })) + .toEqual([{ + coTeacherEmail: 'a@example.com', resourceKey: 'group#g1', + resourceType: 'group', resourceId: 'g1', + }]); + }); + + test('共同管理者がいない / 壊れた行は無視する', () => { + expect(indexRowsForResource('assignment', { classroomId: 'c1' })).toEqual([]); + expect(indexRowsForResource('assignment', { classroomId: 'c1', coTeacherEmails: [] })).toEqual([]); + expect(indexRowsForResource('assignment', { classroomId: 'c1', coTeacherEmails: ['', ' ', 42] })).toEqual([]); + // id が無い行はキーを作れない（索引に入れても引けない）。 + expect(indexRowsForResource('group', { coTeacherEmails: ['a@example.com'] })).toEqual([]); + }); + + test('アーカイブ済みの資源も索引に載せる', () => { + // アーカイブ一覧 (?includeArchived=1) からも共同管理者に見えるべきで、 + // 落とすのは TTL（索引行は資源の ttl を写す）に任せる。 + expect(indexRowsForResource('assignment', { + classroomId: 'c1', status: 'archived', coTeacherEmails: ['a@example.com'], + })).toHaveLength(1); + }); + + test('同じ email が重複していても 1 行だけ作る', () => { + expect(indexRowsForResource('assignment', { + classroomId: 'c1', coTeacherEmails: ['a@example.com', 'A@EXAMPLE.COM'], + })).toHaveLength(1); + }); +}); + +describe('chunkIndexWrites', () => { + test('25 件ずつに割る', () => { + const rows = Array.from({ length: 60 }, (_, i) => ({ + coTeacherEmail: `t${i}@example.com`, resourceKey: 'assignment#c1', + resourceType: 'assignment' as const, resourceId: 'c1', + })); + expect(chunkIndexWrites(rows).map(c => c.length)).toEqual([25, 25, 10]); + expect(BATCH_WRITE_LIMIT).toBe(25); + }); + + test('同一キーの重複は落とす（BatchWrite が ValidationException を返すため）', () => { + const row = { + coTeacherEmail: 'a@example.com', resourceKey: 'assignment#c1', + resourceType: 'assignment' as const, resourceId: 'c1', + }; + expect(chunkIndexWrites([row, { ...row }])).toEqual([[row]]); + }); + + test('同じ email でも課題と組は別キーなので両方残す', () => { + const chunks = chunkIndexWrites([ + { coTeacherEmail: 'a@example.com', resourceKey: 'assignment#c1', resourceType: 'assignment', resourceId: 'c1' }, + { coTeacherEmail: 'a@example.com', resourceKey: 'group#g1', resourceType: 'group', resourceId: 'g1' }, + ]); + expect(chunks[0]).toHaveLength(2); + }); +}); + +describe('parseBackfillArgs / resourceNamesForStage', () => { + test('既定は dry-run', () => { + expect(parseBackfillArgs([])).toEqual({ apply: false }); + expect(parseBackfillArgs(['--apply'])).toEqual({ apply: true }); + }); + + test('索引テーブル名は prod だけ無印', () => { + expect(resourceNamesForStage('prod').coTeacherIndexTable).toBe('ClassroomCoTeacherIndex'); + expect(resourceNamesForStage('stg').coTeacherIndexTable).toBe('ClassroomCoTeacherIndex-stg'); + }); + + test('email の正規化は handler 側と同じ（読み書きでキーが揃う必要がある）', () => { + expect(normalizeIndexEmail(' Foo.Bar@Example.COM ')).toBe('foo.bar@example.com'); + }); +}); diff --git a/infra/smalruby-classroom/lambda/tests/handler-archive-recovery.test.ts b/infra/smalruby-classroom/lambda/tests/handler-archive-recovery.test.ts index 910ae38c931..35c441fdef0 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-archive-recovery.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-archive-recovery.test.ts @@ -19,9 +19,6 @@ export {}; * guard). */ -// モジュール化。トップレベル宣言をファイルスコープに閉じる（他のテストファイルと -// 同名の mockSend / DEV_TOKEN / makeEvent があり、スクリプト扱いだと TS2451 で衝突する）。 -export {}; // Replace only the DynamoDB document client's send(); keep the real command // classes so the handler can still build Get/Update/Query/Scan commands. @@ -300,3 +297,4 @@ describe('archive recoverability (issue #1050)', () => { }); }); }); + diff --git a/infra/smalruby-classroom/lambda/tests/handler-co-teacher-authz.test.ts b/infra/smalruby-classroom/lambda/tests/handler-co-teacher-authz.test.ts index 45da5ea3479..6dfea2dca2a 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-co-teacher-authz.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-co-teacher-authz.test.ts @@ -17,9 +17,6 @@ export {}; * a legitimately authenticated teacher but a stranger to the mocked classroom. */ -// モジュール化。トップレベル宣言をファイルスコープに閉じる（他のテストファイルと -// 同名の mockSend / DEV_TOKEN / makeEvent があり、スクリプト扱いだと TS2451 で衝突する）。 -export {}; // Replace only the DynamoDB document client's send(); keep the real command // classes so the handler can still build Get/Update/Query/Scan commands. @@ -136,3 +133,4 @@ describe('co-teacher endpoints — cross-tenant authorization', () => { expect(JSON.parse(res.body as string).coTeacherEmails).toContain('dev-test-teacher@example.com'); }); }); + diff --git a/infra/smalruby-classroom/lambda/tests/handler-coteacher-index-sync.test.ts b/infra/smalruby-classroom/lambda/tests/handler-coteacher-index-sync.test.ts new file mode 100644 index 00000000000..a0a6be413af --- /dev/null +++ b/infra/smalruby-classroom/lambda/tests/handler-coteacher-index-sync.test.ts @@ -0,0 +1,186 @@ + +/** + * 共同管理者の逆引き索引 (ClassroomCoTeacherIndex) の書き込み同期を固定する + * テスト (issue #1146)。 + * + * `GET /classrooms` はこの索引を読んで一覧を組み立てるので、**索引に書かない + * 経路が 1 つでもあると、その共同管理者には資源が見えなくなる**。しかも + * 権限 (`canManageClassroom`) は今も item 上の `coTeacherEmails` を見るため、 + * 直リンクでは操作できてしまい「一覧に出ないだけ」という気付きにくい壊れ方に + * なる。そこで `coTeacherEmails` を書き換える経路すべてを列挙して固定する: + * + * 1. 課題の共同管理者 追加 (POST /classrooms/{id}/co-teachers) + * 2. 課題の共同管理者 削除 (DELETE /classrooms/{id}/co-teachers/{email}) + * 3. 組の共同管理者 更新 (PATCH /classroom-groups/{id}) + * 4. v2 移行で課題の共同管理者を組へ引き上げる (POST /classroom-groups/migrate) + */ + +const mockSend = jest.fn(); +jest.mock('@aws-sdk/lib-dynamodb', () => { + const actual = jest.requireActual('@aws-sdk/lib-dynamodb'); + return { + ...actual, + DynamoDBDocumentClient: { from: () => ({ send: mockSend }) }, + }; +}); + +const DEV_TOKEN = 'test-dev-bypass'; +const SELF_SUB = 'dev-test-teacher'; +const INDEX_TABLE = 'ClassroomCoTeacherIndex'; + +type Row = Record; + +const makeEvent = ( + method: string, + path: string, + pathParameters: Record = {}, + body?: unknown, +) => ({ + requestContext: { http: { method, path, sourceIp: '127.0.0.1' } }, + headers: { authorization: `Bearer ${DEV_TOKEN}`, origin: 'http://localhost:8601' }, + pathParameters, + body: body === undefined ? undefined : JSON.stringify(body), +}); + +/** 最小の in-memory ルーティング。索引への書き込みだけを記録する。 */ +const install = (tables: Record) => { + const indexWrites: { op: 'put' | 'delete'; email: string; resourceKey: string; ttl?: unknown }[] = []; + mockSend.mockImplementation((cmd: { constructor: { name: string }; input: Record }) => { + const name = cmd.constructor.name; + const input = cmd.input; + const rows = tables[String(input.TableName || '')] || []; + + if (name === 'PutCommand' && input.TableName === INDEX_TABLE) { + indexWrites.push({ + op: 'put', + email: input.Item.coTeacherEmail, + resourceKey: input.Item.resourceKey, + ttl: input.Item.ttl, + }); + return Promise.resolve({}); + } + if (name === 'DeleteCommand' && input.TableName === INDEX_TABLE) { + indexWrites.push({ op: 'delete', email: input.Key.coTeacherEmail, resourceKey: input.Key.resourceKey }); + return Promise.resolve({}); + } + if (name === 'GetCommand') { + const [[keyName, keyValue]] = Object.entries(input.Key as Row); + return Promise.resolve({ Item: rows.find((r) => r[keyName] === keyValue) }); + } + if (name === 'QueryCommand') { + const values = input.ExpressionAttributeValues || {}; + const condition = String(input.KeyConditionExpression || ''); + if (condition.includes('teacherSub')) { + return Promise.resolve({ Items: rows.filter((r) => r.teacherSub === values[':ts']) }); + } + if (condition.includes('groupId = :gid')) { + return Promise.resolve({ Items: rows.filter((r) => r.groupId === values[':gid']) }); + } + return Promise.resolve({ Items: [] }); + } + if (name === 'UpdateCommand') { + const [[keyName, keyValue]] = Object.entries(input.Key as Row); + const row = rows.find((r) => r[keyName] === keyValue) || { ...(input.Key as Row) }; + return Promise.resolve({ Attributes: row }); + } + return Promise.resolve({}); + }); + return indexWrites; +}; + +describe('co-teacher reverse index write sync (issue #1146)', () => { + let handler: (event: unknown) => Promise<{ statusCode?: number; body?: string }>; + + beforeEach(() => { + jest.resetModules(); + process.env.DEV_BYPASS_TOKEN = DEV_TOKEN; + process.env.STAGE = 'stg'; + mockSend.mockReset(); + handler = require('../handler').handler; + }); + + test('課題に共同管理者を追加すると索引に行が増える', async () => { + const writes = install({ + Classrooms: [{ + classroomId: 'c1', teacherSub: SELF_SUB, className: 'x', + status: 'active', coTeacherEmails: [], ttl: 1800000000, + }], + }); + + const res = await handler(makeEvent('POST', '/classrooms/c1/co-teachers', { classroomId: 'c1' }, { + email: 'New.Teacher@Example.COM', + })); + expect(res.statusCode).toBe(200); + // 索引のキーは正規化済み email（Query 側も正規化して引くため）。 + expect(writes).toEqual([ + { op: 'put', email: 'new.teacher@example.com', resourceKey: 'assignment#c1', ttl: 1800000000 }, + ]); + }); + + test('課題の共同管理者を外すと索引の行も消える', async () => { + const writes = install({ + Classrooms: [{ + classroomId: 'c1', teacherSub: SELF_SUB, className: 'x', + status: 'active', coTeacherEmails: ['a@example.com', 'b@example.com'], + }], + }); + + const res = await handler(makeEvent( + 'DELETE', '/classrooms/c1/co-teachers/a%40example.com', + { classroomId: 'c1', email: 'a%40example.com' }, + )); + expect(res.statusCode).toBe(200); + expect(writes).toEqual([ + { op: 'delete', email: 'a@example.com', resourceKey: 'assignment#c1' }, + ]); + }); + + test('組の共同管理者を差し替えると差分だけ書く', async () => { + const writes = install({ + ClassroomGroups: [{ + groupId: 'g1', teacherSub: SELF_SUB, name: '1組', year: 2026, + status: 'active', schemaVersion: 2, topics: [], + coTeacherEmails: ['keep@example.com', 'drop@example.com'], ttl: 1900000000, + }], + }); + + const res = await handler(makeEvent('PATCH', '/classroom-groups/g1', { groupId: 'g1' }, { + coTeacherEmails: ['keep@example.com', 'add@example.com'], + })); + expect(res.statusCode).toBe(200); + // 据え置きの keep@ には何も書かない（同じ内容の上書きはしない）。 + expect(writes).toEqual([ + { op: 'put', email: 'add@example.com', resourceKey: 'group#g1', ttl: 1900000000 }, + { op: 'delete', email: 'drop@example.com', resourceKey: 'group#g1' }, + ]); + }); + + test('v2 移行で組へ引き上げた共同管理者も索引に載る', async () => { + // 移行は課題の共同管理者を組へ union する = coTeacherEmails の書き手。 + // ここで同期しないと、移行済みアカウントの共同管理クラスが一覧から消える。 + const writes = install({ + Classrooms: [{ + classroomId: 'c1', teacherSub: SELF_SUB, className: '1組', + assignmentName: '第1回', status: 'active', studentCount: 30, + groupId: 'g1', createdAt: '2026-08-01T00:00:00.000Z', + coTeacherEmails: ['lifted@example.com'], + }], + ClassroomGroups: [{ + groupId: 'g1', teacherSub: SELF_SUB, name: '1組', year: 2026, + status: 'active', schemaVersion: 1, topics: [], studentCount: 30, + coTeacherEmails: [], ttl: 1900000000, + }], + }); + + const res = await handler(makeEvent('POST', '/classroom-groups/migrate')); + expect(res.statusCode).toBe(200); + expect(writes).toEqual([ + { op: 'put', email: 'lifted@example.com', resourceKey: 'group#g1', ttl: 1900000000 }, + ]); + }); +}); + +// import / export を持たないテストファイルは TS の「スクリプト」扱いになり、 +// ts-jest が 1 プロセスで複数のテストを型付けすると `const mockSend` などが +// グローバルスコープで衝突して "Cannot redeclare block-scoped variable" になる。 +export {}; diff --git a/infra/smalruby-classroom/lambda/tests/handler-group-co-teacher.test.ts b/infra/smalruby-classroom/lambda/tests/handler-group-co-teacher.test.ts index 77f6ffae58b..601b68c7e64 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-group-co-teacher.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-group-co-teacher.test.ts @@ -19,9 +19,6 @@ export {}; * canManageGroup / canManageClassroom. */ -// モジュール化。トップレベル宣言をファイルスコープに閉じる（他のテストファイルと -// 同名の mockSend / DEV_TOKEN / makeEvent があり、スクリプト扱いだと TS2451 で衝突する）。 -export {}; const mockSend = jest.fn(); jest.mock('@aws-sdk/lib-dynamodb', () => { @@ -155,10 +152,28 @@ describe('class-level co-teacher authorization (issue #1138)', () => { * runs in which order is an implementation detail). */ const useTables = (allGroups: Record[], allAssignments: Record[]) => { + // 逆引き索引テーブル（#1146）の中身は、資源の coTeacherEmails から導出する + // ＝ dual-write が正しく走った状態を表す。索引を別に書かないことで、 + // 「索引と資源が食い違ったまま通ってしまうテスト」にならないようにする。 + const indexRows = [ + ...allGroups.flatMap((g: any) => ((g.coTeacherEmails || []) as string[]).map(email => ({ + coTeacherEmail: email, resourceKey: `group#${g.groupId}`, + resourceType: 'group', resourceId: g.groupId, + }))), + ...allAssignments.flatMap((a: any) => ((a.coTeacherEmails || []) as string[]).map(email => ({ + coTeacherEmail: email, resourceKey: `assignment#${a.classroomId}`, + resourceType: 'assignment', resourceId: a.classroomId, + }))), + ]; + const rowsOf = (table: string): Record[] => { + if (table === 'ClassroomGroups') return allGroups; + if (table === 'ClassroomCoTeacherIndex') return indexRows; + return allAssignments; + }; mockSend.mockImplementation((cmd: { constructor: { name: string }; input: Record }) => { const name = cmd.constructor.name; const input = cmd.input; - const rows = input.TableName === 'ClassroomGroups' ? allGroups : allAssignments; + const rows = rowsOf(input.TableName); const values = input.ExpressionAttributeValues || {}; if (name === 'GetCommand') { if (input.TableName === 'ClassroomGroups') { @@ -168,25 +183,39 @@ describe('class-level co-teacher authorization (issue #1138)', () => { } return Promise.resolve({}); } - if (name === 'QueryCommand') { - // Every GSI query in the list path keys off teacherSub. - return Promise.resolve({ Items: rows.filter((r: any) => r.teacherSub === values[':ts']) }); + if (name === 'BatchGetCommand') { + const responses: Record[]> = {}; + for (const [table, spec] of Object.entries(input.RequestItems as Record)) { + const target = rowsOf(table); + responses[table] = spec.Keys + .map(key => { + const [[keyName, keyValue]] = Object.entries(key); + return target.find((r: any) => r[keyName] === keyValue); + }) + .filter((r): r is Record => !!r); + } + return Promise.resolve({ Responses: responses }); } - if (name === 'ScanCommand') { - const filter: string = input.FilterExpression || ''; - if (filter.startsWith('contains(coTeacherEmails')) { - // DynamoDB's contains() is an exact, case-sensitive element - // match — do not soften it here, or the mock would certify - // a case-insensitivity the real table does not have. + if (name === 'QueryCommand') { + const condition: string = input.KeyConditionExpression || ''; + // 逆引き索引: email → 課題 / 組（#1146） + if (condition.includes('coTeacherEmail')) { return Promise.resolve({ - Items: rows.filter((r: any) => (r.coTeacherEmails || []).includes(values[':email'])), + Items: rows.filter((r: any) => r.coTeacherEmail === values[':email'] + && String(r.resourceKey).startsWith(values[':prefix'])), }); } - if (filter.startsWith('groupId IN')) { - const wanted = Object.values(values); - return Promise.resolve({ Items: rows.filter((r: any) => wanted.includes(r.groupId)) }); + // groupId-index: クラスに属する課題（#1146） + if (condition.includes('groupId = :gid')) { + return Promise.resolve({ Items: rows.filter((r: any) => r.groupId === values[':gid']) }); } - return Promise.resolve({ Items: rows }); + // 残りの GSI クエリは teacherSub 起点。 + return Promise.resolve({ Items: rows.filter((r: any) => r.teacherSub === values[':ts']) }); + } + if (name === 'ScanCommand') { + // #1146 で一覧経路の Scan は全廃した。ここに来たら退行なので落とす + // （黙って空を返すと「取りこぼしても green」になってしまう）。 + throw new Error(`unexpected Scan on ${input.TableName} in the list path (#1146)`); } return Promise.resolve({}); }); @@ -316,3 +345,4 @@ describe('canManageGroup / canManageClassroom email normalization (issue #1138)' expect(mod.canManageClassroom(classroom, { sub: 'x', email: 'other@example.com' })).toBe(false); }); }); + diff --git a/infra/smalruby-classroom/lambda/tests/handler-group-enumeration.test.ts b/infra/smalruby-classroom/lambda/tests/handler-group-enumeration.test.ts index eeb0c2d1420..895c84125bb 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-group-enumeration.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-group-enumeration.test.ts @@ -121,6 +121,13 @@ const installTables = (tables: Record) => { const condition = String(input.KeyConditionExpression || ''); let items = rows.filter(row => { if (condition.includes('teacherSub')) return row.teacherSub === values[':ts']; + // groupId-index: クラスに属する課題（#1146 で Scan から置換） + if (condition.includes('groupId = :gid')) return row.groupId === values[':gid']; + // 共同管理者の逆引き索引（#1146） + if (condition.includes('coTeacherEmail')) { + return row.coTeacherEmail === values[':email'] + && String(row.resourceKey || '').startsWith(String(values[':prefix'])); + } if (condition.includes('joinCode')) return row.joinCode === values[':jc']; if (condition.includes('sessionToken')) return row.sessionToken === values[':st']; if (condition.includes('classroomId')) { @@ -138,6 +145,20 @@ const installTables = (tables: Record) => { } return { Items: items }; } + if (name === 'BatchGetCommand') { + const requested = Object.entries(input.RequestItems as Record); + const responses: Record = {}; + for (const [requestedTable, spec] of requested) { + const target = tables[requestedTable] || []; + responses[requestedTable] = spec.Keys + .map(key => { + const [[keyName, keyValue]] = Object.entries(key); + return target.find(row => row[keyName] === keyValue); + }) + .filter((row): row is Item => !!row); + } + return { Responses: responses }; + } if (name === 'PutCommand') { puts.push(input.Item as Item); return {}; diff --git a/infra/smalruby-classroom/lambda/tests/handler-group-seatcount.test.ts b/infra/smalruby-classroom/lambda/tests/handler-group-seatcount.test.ts index 7c58865a7b8..3880b2562f7 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-group-seatcount.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-group-seatcount.test.ts @@ -10,9 +10,6 @@ export {}; * warns before shrinking). Regression for "既存の課題の人数が増えない". */ -// モジュール化。トップレベル宣言をファイルスコープに閉じる（他のテストファイルと -// 同名の mockSend / DEV_TOKEN / makeEvent があり、スクリプト扱いだと TS2451 で衝突する）。 -export {}; const mockSend = jest.fn(); jest.mock('@aws-sdk/lib-dynamodb', () => { @@ -65,8 +62,9 @@ describe('class seat-count propagation (PATCH /classroom-groups/{id})', () => { if (name === 'UpdateCommand' && table.includes('Group')) { return { Attributes: { groupId: 'g1', teacherSub: 'dev-test-teacher', name: '2年1組', year: 2026, studentCount: newCount, schemaVersion: 2 } }; } - if (name === 'ScanCommand') { - // propagation enumerates the class's assignments by groupId (#1145) + if (name === 'QueryCommand') { + // propagation enumerates the class's assignments through the + // groupId-index GSI (#1145 で groupId 起点に、#1146 で Scan から Query に) return { Items: classroomIds.map(classroomId => ({ classroomId, groupId: 'g1', status: 'active' })), }; @@ -121,12 +119,9 @@ describe('class seat-count propagation (PATCH /classroom-groups/{id})', () => { if (name === 'UpdateCommand' && String(command.input?.TableName).includes('Group')) { return { Attributes: { groupId: 'g1', teacherSub: 'dev-test-teacher', studentCount: 40 } }; } - if (name === 'ScanCommand') { + if (name === 'QueryCommand') { const values = (command.input?.ExpressionAttributeValues || {}) as Record; - const wanted = Object.entries(values) - .filter(([key]) => key.startsWith(':g')) - .map(([, value]) => value); - return { Items: rows.filter(row => wanted.includes(row.groupId)) }; + return { Items: rows.filter(row => row.groupId === values[':gid']) }; } if (name === 'UpdateCommand') { touched.push(String((command.input?.Key as Record).classroomId)); @@ -138,4 +133,48 @@ describe('class seat-count propagation (PATCH /classroom-groups/{id})', () => { await handler(makeEvent('PATCH', '/classroom-groups/g1', { groupId: 'g1' }, { studentCount: 40 })); expect(touched).toEqual(['c1']); }); + + test('LastEvaluatedKey が返るときも全ページ分の課題を更新する (#1146)', async () => { + // 1MB 上限は groupId フィルタの「前」に効くので、ページを辿らないと + // クラス内の一部の課題だけ古い人数のまま残る（エラーは出ない）。 + // 列挙は groupId 起点（#1145 で teacherSub 起点から変更）で、#1146 で + // groupId-index の Query になったので、ページングも Query で確認する。 + // status の絞り込みは DynamoDB のフィルタから JS 側へ移ったので、行にも持たせる。 + const rows = [ + { classroomId: 'c1', groupId: 'g1', status: 'active' }, + { classroomId: 'c2', groupId: 'g1', status: 'active' }, + { classroomId: 'c3', groupId: 'g1', status: 'active' }, + ]; + const classroomUpdates: string[] = []; + mockSend.mockImplementation(async (command: { + constructor: { name: string }; input?: Record; + }) => { + const name = command.constructor.name; + const table = String(command.input?.TableName || ''); + if (name === 'GetCommand') { + return { Item: { groupId: 'g1', teacherSub: 'dev-test-teacher', studentCount: 30, schemaVersion: 2 } }; + } + if (name === 'UpdateCommand' && table.includes('Group')) { + return { Attributes: { groupId: 'g1', teacherSub: 'dev-test-teacher', studentCount: 33 } }; + } + if (name === 'QueryCommand') { + const start = (command.input?.ExclusiveStartKey as { index?: number } | undefined)?.index || 0; + const end = Math.min(start + 1, rows.length); + return { + Items: rows.slice(start, end), + ...(end < rows.length ? { LastEvaluatedKey: { index: end } } : {}), + }; + } + if (name === 'UpdateCommand') { + classroomUpdates.push(String((command.input?.Key as Record).classroomId)); + return {}; + } + return {}; + }); + + const res = await handler(makeEvent('PATCH', '/classroom-groups/g1', { groupId: 'g1' }, { studentCount: 33 })); + expect(res.statusCode).toBe(200); + expect(classroomUpdates).toEqual(['c1', 'c2', 'c3']); + }); }); + diff --git a/infra/smalruby-classroom/lambda/tests/handler-list-classrooms-reads.test.ts b/infra/smalruby-classroom/lambda/tests/handler-list-classrooms-reads.test.ts new file mode 100644 index 00000000000..8fe70c94a64 --- /dev/null +++ b/infra/smalruby-classroom/lambda/tests/handler-list-classrooms-reads.test.ts @@ -0,0 +1,307 @@ + +/** + * `GET /classrooms` の読み取り構成を固定するテスト (issue #1146)。 + * + * 3 つの性質を守る: + * + * 1. **Scan を使わない** — 一覧経路から Classrooms / ClassroomGroups への Scan を + * 全廃した。RCU も 1MB 上限もフィルタ適用「前」に効くため、Scan のコストは + * 「その先生に見える量」ではなく「テーブル全体の量」に比例して増える。 + * 共同管理は逆引き索引テーブル（email → 資源）、クラス経由の課題は + * Classrooms の `groupId-index` GSI で引く。 + * 2. **ページング** — 索引を引く Query も `LastEvaluatedKey` を辿らなければ + * エラー無しで取りこぼす。一覧系の Query は最後のページまで辿る。 + * 3. **索引を信用しすぎない** — 逆引き索引は資源本体と結果整合なので、 + * 引いた資源の `coTeacherEmails` を読み直して権限を再確認する + * （認可の真実は今も item 上のリストであり、索引ではない）。 + */ + +const mockSend = jest.fn(); +jest.mock('@aws-sdk/lib-dynamodb', () => { + const actual = jest.requireActual('@aws-sdk/lib-dynamodb'); + return { + ...actual, + DynamoDBDocumentClient: { from: () => ({ send: mockSend }) }, + }; +}); + +const DEV_TOKEN = 'test-dev-bypass'; +const SELF_SUB = 'dev-test-teacher'; +const SELF_EMAIL = 'dev-test-teacher@example.com'; + +const makeEvent = () => ({ + requestContext: { http: { method: 'GET', path: '/classrooms', sourceIp: '127.0.0.1' } }, + headers: { authorization: `Bearer ${DEV_TOKEN}`, origin: 'http://localhost:8601' }, + pathParameters: {}, +}); + +type Row = Record; + +/** + * ページングする in-memory テーブル。`pageSize` を指定すると Query が + * `LastEvaluatedKey` を返し、実装がページを辿らないと一部しか見えない。 + * + * 逆引き索引テーブルの中身は資源の `coTeacherEmails` から導出する + * ＝ dual-write が正しく走った状態を表す。索引を別に与えないことで + * 「索引と資源が食い違ったまま green になるテスト」を避ける。 + */ +const useTables = (groups: Row[], assignments: Row[], pageSize = 0) => { + const indexRows: Row[] = [ + ...groups.flatMap((g) => ((g.coTeacherEmails || []) as string[]).map((email) => ({ + coTeacherEmail: email, resourceKey: `group#${g.groupId}`, + resourceType: 'group', resourceId: g.groupId, + }))), + ...assignments.flatMap((a) => ((a.coTeacherEmails || []) as string[]).map((email) => ({ + coTeacherEmail: email, resourceKey: `assignment#${a.classroomId}`, + resourceType: 'assignment', resourceId: a.classroomId, + }))), + ]; + const rowsOf = (table: string): Row[] => { + if (table === 'ClassroomGroups') return groups; + if (table === 'ClassroomCoTeacherIndex') return indexRows; + return assignments; + }; + + mockSend.mockImplementation((cmd: { constructor: { name: string }; input: Record }) => { + const name = cmd.constructor.name; + const input = cmd.input; + const values = input.ExpressionAttributeValues || {}; + + if (name === 'GetCommand') { + if (input.TableName === 'ClassroomGroups') { + return Promise.resolve({ Item: groups.find((g) => g.groupId === input.Key.groupId) }); + } + return Promise.resolve({}); + } + if (name === 'BatchGetCommand') { + const responses: Record = {}; + for (const [table, spec] of Object.entries(input.RequestItems as Record)) { + expect(spec.Keys.length).toBeLessThanOrEqual(100); + const target = rowsOf(table); + responses[table] = spec.Keys + .map((key) => { + const [[keyName, keyValue]] = Object.entries(key); + return target.find((r) => r[keyName] === keyValue); + }) + .filter((r): r is Row => !!r); + } + return Promise.resolve({ Responses: responses }); + } + if (name !== 'QueryCommand') { + // Scan はもう一覧経路に無い。呼ばれた事実は scansAgainst() で assert する。 + return Promise.resolve({ Items: [] }); + } + + const rows = rowsOf(input.TableName); + const condition: string = input.KeyConditionExpression || ''; + const keyMatch = (r: Row): boolean => { + if (condition.includes('coTeacherEmail')) { + return r.coTeacherEmail === values[':email'] + && String(r.resourceKey).startsWith(values[':prefix']); + } + if (condition.includes('groupId = :gid')) return r.groupId === values[':gid']; + return r.teacherSub === values[':ts']; + }; + // キー条件に合う行だけをページに切り出す（実 Query と同じで、 + // Query は一致するキー範囲のみを読む）。 + const matched = rows.filter(keyMatch); + const start = input.ExclusiveStartKey ? (input.ExclusiveStartKey.index as number) : 0; + const end = pageSize > 0 ? Math.min(start + pageSize, matched.length) : matched.length; + + return Promise.resolve({ + Items: matched.slice(start, end), + ...(end < matched.length ? { LastEvaluatedKey: { index: end } } : {}), + }); + }); +}; + +describe('GET /classrooms — DynamoDB 読み取り構成 (issue #1146)', () => { + let handler: (event: unknown) => Promise<{ statusCode?: number; body?: string }>; + + beforeEach(() => { + jest.resetModules(); + process.env.DEV_BYPASS_TOKEN = DEV_TOKEN; + process.env.STAGE = 'stg'; + mockSend.mockReset(); + handler = require('../handler').handler; + }); + + const commandsOf = (commandName: string, table?: string) => + mockSend.mock.calls + .map((c) => c[0]) + .filter((c) => c?.constructor?.name === commandName + && (table === undefined || c?.input?.TableName === table)); + + const scansAgainst = (table: string) => commandsOf('ScanCommand', table); + + const listIds = async () => { + const res = await handler(makeEvent()); + expect(res.statusCode).toBe(200); + return JSON.parse(res.body || '{}').classrooms.map((c: { classroomId: string }) => c.classroomId).sort(); + }; + + const ownGroup = { groupId: 'g-own', teacherSub: SELF_SUB, name: '1組', status: 'active' }; + const coGroup = { + groupId: 'g-co', + teacherSub: 'other-teacher', + coTeacherEmails: [SELF_EMAIL], + name: '2組', + status: 'active', + }; + + const assignment = (id: string, over: Row = {}): Row => ({ + classroomId: id, + teacherSub: 'other-teacher', + className: 'x', + status: 'active', + ...over, + }); + + test('一覧経路は Classrooms / ClassroomGroups を Scan しない', async () => { + useTables([ownGroup, coGroup], [ + assignment('c-own', { teacherSub: SELF_SUB }), + assignment('c-co-assignment', { coTeacherEmails: [SELF_EMAIL] }), + assignment('c-in-own-group', { groupId: 'g-own' }), + assignment('c-in-co-group', { groupId: 'g-co' }), + assignment('c-foreign', { groupId: 'g-foreign' }), + ]); + + expect(await listIds()).toEqual(['c-co-assignment', 'c-in-co-group', 'c-in-own-group', 'c-own']); + expect(scansAgainst('Classrooms')).toHaveLength(0); + expect(scansAgainst('ClassroomGroups')).toHaveLength(0); + }); + + test('共同管理は逆引き索引、クラス経由は groupId-index で引く', async () => { + useTables([coGroup], [ + assignment('c-co-assignment', { coTeacherEmails: [SELF_EMAIL] }), + assignment('c-in-co-group', { groupId: 'g-co' }), + ]); + + expect(await listIds()).toEqual(['c-co-assignment', 'c-in-co-group']); + + // 逆引き索引は課題用と組用の 2 本（接頭辞で撃ち分ける）。 + const indexQueries = commandsOf('QueryCommand', 'ClassroomCoTeacherIndex'); + expect(indexQueries.map((q) => q.input.ExpressionAttributeValues[':prefix']).sort()) + .toEqual(['assignment#', 'group#']); + for (const query of indexQueries) { + expect(query.input.ExpressionAttributeValues[':email']).toBe(SELF_EMAIL); + } + + // クラス経由の課題は GSI の Query（旧 `groupId IN (...)` の Scan を置換）。 + const groupIdQueries = commandsOf('QueryCommand', 'Classrooms') + .filter((q) => q.input.IndexName === 'groupId-index'); + expect(groupIdQueries.map((q) => q.input.ExpressionAttributeValues[':gid'])).toEqual(['g-co']); + }); + + test('索引が消し忘れを返しても、資源本体のリストで再確認して除外する', async () => { + // 逆引き索引は資源と結果整合。共同管理者から外れた課題の行が残っていても、 + // item 上の coTeacherEmails が真実なので一覧に混ぜてはいけない。 + useTables([], [assignment('c-stale')]); + const stale = { + coTeacherEmail: SELF_EMAIL, resourceKey: 'assignment#c-stale', + resourceType: 'assignment', resourceId: 'c-stale', + }; + const inner = mockSend.getMockImplementation()!; + mockSend.mockImplementation((cmd: any) => { + if (cmd?.constructor?.name === 'QueryCommand' + && cmd.input.TableName === 'ClassroomCoTeacherIndex' + && cmd.input.ExpressionAttributeValues[':prefix'] === 'assignment#') { + return Promise.resolve({ Items: [stale] }); + } + return inner(cmd); + }); + + expect(await listIds()).toEqual([]); + }); + + test('DDB_MAX_PAGES が不正値でも既定にフォールバックして全件返す', async () => { + // 不正値をそのまま上限に使うと 1 ページも読まずに空を返し、 + // ページングが防ごうとしている「黙って取りこぼす」状態になる。 + jest.resetModules(); + process.env.DDB_MAX_PAGES = 'not-a-number'; + handler = require('../handler').handler; + useTables( + [], + [ + assignment('c-1', { teacherSub: SELF_SUB }), + assignment('c-2', { teacherSub: SELF_SUB }), + ], + 1, + ); + + try { + expect(await listIds()).toEqual(['c-1', 'c-2']); + } finally { + delete process.env.DDB_MAX_PAGES; + } + }); + + test('LastEvaluatedKey が返るときは全ページを辿る (Query: 自分の課題)', async () => { + useTables( + [], + [ + assignment('c-1', { teacherSub: SELF_SUB }), + assignment('c-2', { teacherSub: SELF_SUB }), + assignment('c-3', { teacherSub: SELF_SUB }), + ], + 1, + ); + + expect(await listIds()).toEqual(['c-1', 'c-2', 'c-3']); + }); + + test('LastEvaluatedKey が返るときは全ページを辿る (逆引き索引 / groupId-index)', async () => { + // 1 ページ 1 件しか返らないので、辿らなければ 1 件しか見えない。 + useTables( + [ownGroup, coGroup], + [ + assignment('c-1', { coTeacherEmails: [SELF_EMAIL] }), + assignment('c-2', { coTeacherEmails: [SELF_EMAIL] }), + assignment('c-3', { groupId: 'g-own' }), + assignment('c-4', { groupId: 'g-own' }), + assignment('c-5', { groupId: 'g-co' }), + ], + 1, + ); + + expect(await listIds()).toEqual(['c-1', 'c-2', 'c-3', 'c-4', 'c-5']); + }); + + test('共同管理クラスが 100 件を超えても BatchGet を分割して全件拾う', async () => { + // 旧実装の `groupId IN (...)` は 100 オペランド上限でチャンク分割していた。 + // 索引経由になったので、上限は BatchGetItem の 100 キー（mock 側で assert）。 + const manyGroups = Array.from({ length: 150 }, (_, i) => ({ + groupId: `g${i}`, + teacherSub: 'other-teacher', + coTeacherEmails: [SELF_EMAIL], + status: 'active', + })); + useTables(manyGroups, [assignment('c-first', { groupId: 'g0' }), assignment('c-last', { groupId: 'g149' })]); + + expect(await listIds()).toEqual(['c-first', 'c-last']); + expect(commandsOf('BatchGetCommand')).toHaveLength(2); + expect(scansAgainst('ClassroomGroups')).toHaveLength(0); + }); + + test('無限ループ防止のページ上限で打ち切る', async () => { + // LastEvaluatedKey を返し続ける壊れたテーブル。 + mockSend.mockImplementation((cmd: { constructor: { name: string } }) => { + const name = cmd.constructor.name; + if (name === 'QueryCommand' || name === 'ScanCommand') { + return Promise.resolve({ Items: [], LastEvaluatedKey: { index: 1 } }); + } + return Promise.resolve({}); + }); + + const res = await handler(makeEvent()); + expect(res.statusCode).toBe(200); + // 打ち切られること（＝ハングしない）が要件。上限は実装定数。 + expect(mockSend.mock.calls.length).toBeLessThan(500); + }); +}); + +// import / export を持たないテストファイルは TS の「スクリプト」扱いになり、 +// ts-jest が 1 プロセスで複数のテストを型付けすると `const mockSend` などが +// グローバルスコープで衝突して "Cannot redeclare block-scoped variable" になる。 +// 空 export でモジュール化してファイルごとのスコープに閉じる。 +export {}; diff --git a/infra/smalruby-classroom/lambda/tests/handler-notifications.test.ts b/infra/smalruby-classroom/lambda/tests/handler-notifications.test.ts index 3242b37c654..9001f047d8a 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-notifications.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-notifications.test.ts @@ -10,9 +10,6 @@ export {}; * written by the admin stack, so there is no create path here. */ -// モジュール化。トップレベル宣言をファイルスコープに閉じる（他のテストファイルと -// 同名の mockSend / DEV_TOKEN / makeEvent があり、スクリプト扱いだと TS2451 で衝突する）。 -export {}; const mockSend = jest.fn(); jest.mock('@aws-sdk/lib-dynamodb', () => { @@ -223,3 +220,4 @@ describe('お知らせセンター (EPIC #1111)', () => { }); }); }); + diff --git a/infra/smalruby-classroom/lambda/tests/handler-shared-assignments.test.ts b/infra/smalruby-classroom/lambda/tests/handler-shared-assignments.test.ts index 9dd3309b135..214cc006ba3 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-shared-assignments.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-shared-assignments.test.ts @@ -10,9 +10,6 @@ export {}; * Design canon: spike #1067 (D1-D12). */ -// モジュール化。トップレベル宣言をファイルスコープに閉じる（他のテストファイルと -// 同名の mockSend / DEV_TOKEN / makeEvent があり、スクリプト扱いだと TS2451 で衝突する）。 -export {}; const mockSend = jest.fn(); jest.mock('@aws-sdk/lib-dynamodb', () => { @@ -657,3 +654,4 @@ describe('みんなの課題 (issue #1068)', () => { }); }); }); + diff --git a/infra/smalruby-classroom/lambda/tests/handler-shared-recommend-projection.test.ts b/infra/smalruby-classroom/lambda/tests/handler-shared-recommend-projection.test.ts index 48d8b8d7013..b008e9479da 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-shared-recommend-projection.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-shared-recommend-projection.test.ts @@ -11,9 +11,6 @@ export {}; * 巻き戻らない（レビュー指摘の回帰テスト） */ -// モジュール化。トップレベル宣言をファイルスコープに閉じる（他のテストファイルと -// 同名の mockSend / DEV_TOKEN / makeEvent があり、スクリプト扱いだと TS2451 で衝突する）。 -export {}; const mockSend = jest.fn(); jest.mock('@aws-sdk/lib-dynamodb', () => { @@ -175,3 +172,4 @@ describe('先生側 API と Admin 推薦の境界 (#1110)', () => { expect(res.body).not.toContain('recommendedBy'); }); }); + diff --git a/infra/smalruby-classroom/lib/classroom-stack.ts b/infra/smalruby-classroom/lib/classroom-stack.ts index 138c24fd276..f402b685425 100644 --- a/infra/smalruby-classroom/lib/classroom-stack.ts +++ b/infra/smalruby-classroom/lib/classroom-stack.ts @@ -21,6 +21,7 @@ export class ClassroomStack extends cdk.Stack { public readonly submissionsTable: dynamodb.Table; public readonly kickRequestsTable: dynamodb.Table; public readonly groupsTable: dynamodb.Table; + public readonly coTeacherIndexTable: dynamodb.Table; public readonly sharedAssignmentsTable: dynamodb.Table; public readonly sharedReportsTable: dynamodb.Table; public readonly notificationsTable: dynamodb.Table; @@ -115,6 +116,19 @@ export class ClassroomStack extends cdk.Stack { projectionType: dynamodb.ProjectionType.ALL, }); + // GSI: groupId lookup — every assignment filed under a class (組), whoever + // created it. Replaces the `groupId IN (...)` Scan of the whole table + // (#1146): a Scan is charged on the items read *before* the filter, so it + // grows with the table, while this query only reads the matching rows. + this.classroomsTable.addGlobalSecondaryIndex({ + indexName: 'groupId-index', + partitionKey: { + name: 'groupId', + type: dynamodb.AttributeType.STRING, + }, + projectionType: dynamodb.ProjectionType.ALL, + }); + cdk.Tags.of(this.classroomsTable).add('ResourceType', 'DynamoDB'); // Memberships table @@ -256,6 +270,37 @@ export class ClassroomStack extends cdk.Stack { cdk.Tags.of(this.groupsTable).add('ResourceType', 'DynamoDB'); + // Co-teacher reverse index (#1146). DynamoDB cannot index a list + // attribute, so "which assignments / classes is this email a co-teacher + // of?" used to be answered by scanning the whole table with + // `contains(coTeacherEmails, :email)` — a read that grows with the table + // even though the answer is a handful of rows. This table keeps one row + // per (co-teacher email × resource), written whenever a co-teacher list + // changes, so the lookup becomes a Query on the email. + // + // Rows carry the ttl of the resource they point at, so they expire with + // it. A stale row is harmless: the reader fetches the real item and skips + // the ones that are gone. + this.coTeacherIndexTable = new dynamodb.Table(this, 'CoTeacherIndexTable', { + tableName: `ClassroomCoTeacherIndex${stageSuffix}`, + partitionKey: { + name: 'coTeacherEmail', + type: dynamodb.AttributeType.STRING, + }, + sortKey: { + name: 'resourceKey', + type: dynamodb.AttributeType.STRING, + }, + billingMode: dynamodb.BillingMode.PAY_PER_REQUEST, + removalPolicy: cdk.RemovalPolicy.DESTROY, + pointInTimeRecoverySpecification: { + pointInTimeRecoveryEnabled: false, + }, + timeToLiveAttribute: 'ttl', + }); + + cdk.Tags.of(this.coTeacherIndexTable).add('ResourceType', 'DynamoDB'); + // --- みんなの課題 (shared assignment library, EPIC #1066) --- // Shared items are nationwide teacher contributions: permanent (no TTL) // and RETAINed in prod so a stack replacement can never destroy them @@ -440,6 +485,7 @@ export class ClassroomStack extends cdk.Stack { SUBMISSIONS_TABLE_NAME: this.submissionsTable.tableName, KICK_REQUESTS_TABLE_NAME: this.kickRequestsTable.tableName, GROUPS_TABLE_NAME: this.groupsTable.tableName, + CO_TEACHER_INDEX_TABLE_NAME: this.coTeacherIndexTable.tableName, SHARED_ASSIGNMENTS_TABLE_NAME: this.sharedAssignmentsTable.tableName, SHARED_REPORTS_TABLE_NAME: this.sharedReportsTable.tableName, NOTIFICATIONS_TABLE_NAME: this.notificationsTable.tableName, @@ -458,6 +504,10 @@ export class ClassroomStack extends cdk.Stack { PRESIGNED_URL_DOWNLOAD_EXPIRY: process.env.PRESIGNED_URL_DOWNLOAD_EXPIRY || '3600', JOIN_RATE_LIMIT_WINDOW_SECONDS: process.env.JOIN_RATE_LIMIT_WINDOW_SECONDS || '60', JOIN_RATE_LIMIT_MAX_ATTEMPTS: process.env.JOIN_RATE_LIMIT_MAX_ATTEMPTS || '50', + // Page cap for the paginated list reads (#1146). The Lambda env is an + // explicit allowlist, so this has to be here for the documented knob + // to actually be settable from .env.. + DDB_MAX_PAGES: process.env.DDB_MAX_PAGES || '25', // AI evaluation support (empty key disables the endpoint with 503) ANTHROPIC_API_KEY: process.env.ANTHROPIC_API_KEY || '', CLAUDE_MODEL: process.env.CLAUDE_MODEL || 'claude-haiku-4-5-20251001', @@ -479,6 +529,7 @@ export class ClassroomStack extends cdk.Stack { this.submissionsTable.grantReadWriteData(handlerFn); this.kickRequestsTable.grantReadWriteData(handlerFn); this.groupsTable.grantReadWriteData(handlerFn); + this.coTeacherIndexTable.grantReadWriteData(handlerFn); this.sharedAssignmentsTable.grantReadWriteData(handlerFn); this.sharedReportsTable.grantReadWriteData(handlerFn); // お知らせは admin スタックが単一の書き手 (#1111)。この Lambda は一覧