diff --git a/CLAUDE.md b/CLAUDE.md index 34551be..ca35f35 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -116,6 +116,13 @@ These cost real time to discover; `docs/API.md` has the full list with evidence. clamps and `listAllCourses` pages for you. - There is no `GET /tasks/{id}`, and the task lists omit `description` — it only exists on the course page's task element. `get_task` does that join. +- **`GET /cards?ids=` takes at most 20 ids** (the `qs` `arrayLimit` default), and + fails above that with a validation error that blames the ids rather than their + number. `MAX_IDS_PER_QUERY` in `core/client.ts`. Any board over 20 cards is + affected, which is common. +- **Never swallow a per-item crawl error.** Board failures used to be caught and + dropped, so the index lost whole boards while the crawl reported success — + which is how the 20-id limit went unnoticed. They go into `Snapshot.failures`. - **Board file elements carry no file id.** Files are found by listing files-storage with `parentType: 'boardnodes'` and the *element* id as `parentId`. Same for `fileFolder` and `drawing`. diff --git a/docs/API.md b/docs/API.md index fd948d3..84fcf4f 100644 --- a/docs/API.md +++ b/docs/API.md @@ -109,6 +109,16 @@ files-storage with `parentType: 'boardnodes'` and the **element** id as `parentId`. This is the single least discoverable part of the API, and applies equally to `fileFolder` and `drawing` elements. +**`GET /cards?ids=` accepts at most 20 ids.** Above that the request fails with +`400 "each value in ids must be a mongodb id"` — blaming the ids when the real +problem is how many there are. Express/NestJS parse the query string with `qs`, +whose default `arrayLimit` is 20; past it, repeated params become an object +keyed `"0"`, `"1"`, … and `@IsMongoId({ each: true })` then rejects every value. +Nothing in the controller or its DTO says so: the limit lives in the query +parser underneath them. Verified live — 20 ids return 200, 21 return 400 with +identical ids. A board with more than 20 cards is therefore unreadable in one +request; `getCards` chunks at 20. + **`storageLocationId` is the school id** (from `/me`), with `storageLocation: 'school'`, for every parent type in normal use. diff --git a/src/core/client.ts b/src/core/client.ts index 21dd834..8ef358d 100644 --- a/src/core/client.ts +++ b/src/core/client.ts @@ -15,6 +15,12 @@ import type { TaskContent, } from './types.ts'; +/** + * Maximum repeated query parameters the API's parser will still treat as an + * array — the `qs` default. See `getCards` for what happens above it. + */ +export const MAX_IDS_PER_QUERY = 20; + /** Statuses worth retrying: transient by definition, and every call here is a GET. */ const RETRYABLE = new Set([429, 500, 502, 503, 504]); const MAX_RETRIES = 3; @@ -296,12 +302,22 @@ export class SchulcloudClient { } /** - * Card bodies for the given ids. The upstream endpoint takes repeated - * `ids` query params with no documented ceiling, so we chunk purely to - * keep request URLs a sane length. + * Card bodies for the given ids. + * + * **Never request more than 20 at once.** Express/NestJS parse the query + * string with `qs`, whose default `arrayLimit` is 20: past that, repeated + * `ids=` params stop becoming an array and become an object keyed `"0"`, + * `"1"`, … `@IsMongoId({ each: true })` then iterates something that is not + * an array and rejects every value, so the API answers + * `"each value in ids must be a mongodb id"` — blaming the ids when the real + * problem is how many there are. Verified against the live instance: 20 ids + * return 200, 21 return 400, with identical ids. + * + * Nothing in the controller or its DTO says this; the limit lives in the + * query parser underneath them. */ async getCards(cardIds: string[]): Promise { - const CHUNK = 40; + const CHUNK = MAX_IDS_PER_QUERY; const out: CardResponse[] = []; for (let i = 0; i < cardIds.length; i += CHUNK) { const chunk = cardIds.slice(i, i + CHUNK); diff --git a/src/core/crawl.ts b/src/core/crawl.ts index 71491dc..600bc85 100644 --- a/src/core/crawl.ts +++ b/src/core/crawl.ts @@ -74,8 +74,12 @@ export interface Snapshot { schoolId: string; courses: CrawledCourse[]; files: CrawledFile[]; - /** Courses whose page could not be read, with the reason. */ - failures: { courseId: string; reason: string }[]; + /** + * Anything that could not be read, with the reason. Boards appear here too: + * a board that fails must not vanish silently, or the index quietly loses + * content while reporting success. + */ + failures: { courseId: string; boardId?: string; reason: string }[]; } export interface CrawlOptions { @@ -101,7 +105,7 @@ export async function crawl(client: SchulcloudClient, options: CrawlOptions): Pr const crawled: CrawledCourse[] = []; const files: CrawledFile[] = []; - const failures: { courseId: string; reason: string }[] = []; + const failures: { courseId: string; boardId?: string; reason: string }[] = []; let done = 0; await forEachLimited(courses, options.courseConcurrency ?? 5, async (course) => { @@ -109,6 +113,7 @@ export async function crawl(client: SchulcloudClient, options: CrawlOptions): Pr const result = await crawlCourse(client, course, options, includeFiles, includeLessons); crawled.push(result.course); files.push(...result.files); + failures.push(...result.failures); } catch (error) { failures.push({ courseId: course.id, reason: error instanceof Error ? error.message : String(error) }); } finally { @@ -130,10 +135,11 @@ async function crawlCourse( options: CrawlOptions, includeFiles: boolean, includeLessons: boolean, -): Promise<{ course: CrawledCourse; files: CrawledFile[] }> { +): Promise<{ course: CrawledCourse; files: CrawledFile[]; failures: { courseId: string; boardId: string; reason: string }[] }> { const page = await client.getCourseBoard(course.id); const title = page.title || course.title; const files: CrawledFile[] = []; + const failures: { courseId: string; boardId: string; reason: string }[] = []; const boards: CrawledBoard[] = []; const lessons: CrawledLesson[] = []; @@ -195,10 +201,20 @@ async function crawlCourse( } await forEachLimited(boardIds, options.boardConcurrency ?? 4, async (boardId) => { - const assembled = await assembleBoard(client, boardId, options.schoolId, { resolveFiles: includeFiles }).catch( - () => undefined, - ); - if (!assembled) return; + let assembled: AssembledBoard; + try { + assembled = await assembleBoard(client, boardId, options.schoolId, { resolveFiles: includeFiles }); + } catch (error) { + // Record rather than swallow: a dropped board used to disappear from the + // index while the crawl still reported success, which is how a 20-card + // query limit went unnoticed. + failures.push({ + courseId: course.id, + boardId, + reason: error instanceof Error ? error.message : String(error), + }); + return; + } const parts: string[] = []; for (const column of assembled.columns) { @@ -234,7 +250,7 @@ async function crawlCourse( }); boards.sort((a, b) => a.id.localeCompare(b.id)); - return { course: { course, title, boards, lessons, tasks }, files }; + return { course: { course, title, boards, lessons, tasks }, files, failures }; } async function listFiles( diff --git a/src/indexer/indexer.ts b/src/indexer/indexer.ts index 9db4eba..102e18c 100644 --- a/src/indexer/indexer.ts +++ b/src/indexer/indexer.ts @@ -27,7 +27,7 @@ export interface IndexResult { mirrored: number; extracted: number; skipped: number; - failures: { courseId: string; reason: string }[]; + failures: { courseId: string; boardId?: string; reason: string }[]; durationMs: number; /** Set when the caller joined a run already in progress. */ joined?: boolean; diff --git a/src/mcp/tools/index-tools.ts b/src/mcp/tools/index-tools.ts index 809713b..adef518 100644 --- a/src/mcp/tools/index-tools.ts +++ b/src/mcp/tools/index-tools.ts @@ -44,7 +44,10 @@ export function registerIndexTools(server: McpServer, context: ServerContext): v `- Newly mirrored: ${result.mirrored}, text extracted: ${result.extracted}, skipped: ${result.skipped}`, `- Took ${(result.durationMs / 1000).toFixed(1)}s`, result.failures.length > 0 - ? `- Could not read ${result.failures.length} course(s): ${result.failures.map((f) => f.courseId).join(', ')}` + ? `- **Could not read ${result.failures.length} item(s)**: ` + + result.failures + .map((f) => (f.boardId ? `board ${f.boardId}` : `course ${f.courseId}`)) + .join(', ') : undefined, ] .filter(Boolean) diff --git a/test/client.test.ts b/test/client.test.ts index b96b6b2..f227b44 100644 --- a/test/client.test.ts +++ b/test/client.test.ts @@ -80,3 +80,32 @@ describe('SchulcloudClient retries', () => { assert.equal(calls(), 4, 'one attempt plus three retries'); }); }); + +describe('getCards chunking', () => { + it('never sends more than 20 ids in one request', async () => { + // The API's query parser turns >20 repeated params into an object, and + // validation then rejects every id with a message blaming the ids rather + // than their number. Verified live: 20 -> 200, 21 -> 400. + const seen: number[] = []; + globalThis.fetch = (async (url: string | URL) => { + const count = [...new globalThis.URL(String(url)).searchParams.getAll('ids')].length; + seen.push(count); + return json({ data: [] }); + }) as typeof fetch; + + const ids = Array.from({ length: 47 }, (_, i) => String(i).padStart(24, '0')); + await new SchulcloudClient(config).getCards(ids); + + assert.deepEqual(seen, [20, 20, 7], 'should split 47 ids into 20/20/7'); + assert.ok(Math.max(...seen) <= 20); + }); + + it('merges the chunked responses into one list', async () => { + let call = 0; + globalThis.fetch = (async () => + json({ data: [{ id: `card${call++}`, height: 1, elements: [] }] })) as typeof fetch; + const ids = Array.from({ length: 25 }, (_, i) => String(i).padStart(24, '0')); + const cards = await new SchulcloudClient(config).getCards(ids); + assert.equal(cards.length, 2, 'one card from each of the two chunks'); + }); +});