From 634b004985e45fe6e53342414f55479857a97126 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 12 Sep 2026 22:37:47 +0200 Subject: [PATCH] Fix get_board on boards with more than 20 cards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/v3/cards?ids= accepts at most 20 ids. Above that the request fails with 400 "each value in ids must be a mongodb id" — which blames 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 the repeated params stop being an array and become an object keyed "0", "1", …, and @IsMongoId({ each: true }) then rejects every value. I had chunked at 40, having read the controller and its DTO and found no documented ceiling. The limit is not there — it is in the query parser underneath them, which I did not think to check. Verified live: 20 ids return 200, 21 return 400 with identical ids. The worse half of this was mine alone. The crawler caught assembleBoard failures and dropped them, so every board over 20 cards vanished from the index while the crawl reported "failures: none". Board errors now go into Snapshot.failures and are surfaced by refresh_index. Impact of both fixes on a full re-crawl: 205 files -> 255, and the reported board (27 cards, 18 files) reads fully. The two failures that remain are genuine 403s — boards this account cannot see — and are now visible rather than silent. Thanks to the bug report, which had the root cause exactly right. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 7 +++++++ docs/API.md | 10 ++++++++++ src/core/client.ts | 24 ++++++++++++++++++++---- src/core/crawl.ts | 34 +++++++++++++++++++++++++--------- src/indexer/indexer.ts | 2 +- src/mcp/tools/index-tools.ts | 5 ++++- test/client.test.ts | 29 +++++++++++++++++++++++++++++ 7 files changed, 96 insertions(+), 15 deletions(-) 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'); + }); +});