test(beacon-node): cover REST query string array parsing at boundary sizes

[lodekeeper]

Motivation

Regression coverage for #9672. After the v1.44.0 qs bump, GET /eth/v1/beacon/states/{state_id}/validators?id=a,b,c,... with more than 20 comma-separated ids started failing with id must be array (400): qs's default arrayLimit (20) is below the beacon-API maxItems (64) for the validator id query, so anything over 20 items was parsed as an object instead of an array and rejected by the type: "array" schema. #9673 fixed it by raising the querystring arrayLimit to NUMBER_OF_COLUMNS, but no test guards the behavior.

This is PR 1 of 2 for the ask in #9672 (unit coverage that would have caught the regression); a separate PR adds e2e coverage against a real running node.

Description

Adds a RestApiServer-level unit test that exercises query-string array parsing at representative boundary sizes.

Key point: the test drives the real RestApiServer, so it is bound to the production querystring parser configured in base.ts (rather than a stand-alone Fastify instance with a copied parser config). If the arrayLimit override regresses, the 21/64/65-item cases fail exactly as #9672 did.

• Covers the validator id query (Schema.UintOrStringArray, maxItems=64) and the data-column indices query (Schema.UintArray, maxItems=NUMBER_OF_COLUMNS).
• Representative boundary sizes — 1, 20, 21, 64, 65, NUMBER_OF_COLUMNS — for both comma-separated (?id=a,b,c) and repeated (?id=a&id=b) wire forms, rather than exhaustively enumerating 1..64.
• Asserts that more than 64 items are still accepted (65 passes): Lodestar intentionally does not hard-cap validator id arrays server-side at the spec maxItems. The only cap is the qs arrayLimit (NUMBER_OF_COLUMNS), beyond which qs returns an object and schema validation rejects it (129 → 400 with the exact id must be array message from getStateValidators returns 400 "id must be array" for >20 comma-separated id values (regression in v1.44.0) #9672).

Test-only change; no source behavior is modified.

🤖 Generated with AI assistance

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 25a81582bd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Add messages to assertions inside the boundary loop

The assertions in both test cases generated by this loop omit assertion messages. Add messages identifying the wire form and boundary count so failures retain the required context; repository guidance explicitly requires assertion messages for looped or repeated assertions.

AGENTS.md reference: AGENTS.md:L294-L299

Useful? React with 👍 / 👎.

[lodekeeper]

Fixed in 9d90312 — added diagnostic messages naming the wire form (comma-separated / repeated) and item count to each assertion inside the ACCEPTED_COUNTS boundary loop, per the AGENTS.md looped-assertion convention:

expect(res.statusCode, `comma-separated id query with ${n} items should be accepted (200)`).toBe(200);
expect(Array.isArray(id), `comma-separated id query with ${n} items should parse as an array`).toBe(true);
expect(id, `comma-separated id query should preserve all ${n} parsed items`).toHaveLength(n);

(and the same for the repeated-form it).

[nflaig]

can we close this since we merged #9713? those should cover the same cases just with a different testing strategy, merging both prs may be redundant

[nflaig]

@lodekeeper

[lodekeeper]

Compared the two — they overlap heavily, but #9712 isn't a strict duplicate. Breakdown:

Both cover: validator id at 64 and NUMBER_OF_COLUMNS, comma + repeat form. So the #9672 regression itself (querystring arrayLimit < spec maxItems) is safely guarded by #9713 alone — agree there.

Only in #9712 (unit):

• Exact regression boundary — 20 (last count OK under the old default limit) and 21 (first count that broke in getStateValidators returns 400 "id must be array" for >20 comma-separated id values (regression in v1.44.0) #9672). test(beacon-node): add e2e beacon-api query array length compliance tests #9713 jumps straight to 64.
• 1-item scalar→array coercion, and 65 (above spec maxItems, still accepted — pins that we don't hard-cap at maxItems).
• Overflow negative case: NUMBER_OF_COLUMNS + 1 → 400 "must be array". test(beacon-node): add e2e beacon-api query array length compliance tests #9713 has no negative / upper-bound assertion.
• The getDebugDataColumnSidecars indices param (Schema.UintArray). test(beacon-node): add e2e beacon-api query array length compliance tests #9713's own (merged) docstring notes this path "requires a Fulu block with columns to exercise end-to-end and is covered at the unit level," and that "the unit coverage in the sibling PR pins the parser at the RestApiServer level" — i.e. test(beacon-node): add e2e beacon-api query array length compliance tests #9713 explicitly defers this to test(beacon-node): cover REST query string array parsing at boundary sizes #9712.

So closing #9712 would drop the indices-param + overflow + exact-boundary cases and leave #9713's docstring referencing a closed sibling.

My lean: keep #9712 as the unit half of the pair — it's cheap (no node spin-up) and covers what the e2e suite deliberately doesn't. If you'd rather not maintain two files, I can instead trim the overlapping 64/NUMBER_OF_COLUMNS id cases so #9712 holds only the unit-unique cases. Whichever you prefer — happy to close if you still want to consolidate.