Fix get_board on boards with more than 20 cards
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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`.
|
||||
|
||||
10
docs/API.md
10
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.
|
||||
|
||||
|
||||
@@ -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<CardResponse[]> {
|
||||
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);
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user