fix(metadata-protocol): check query-parameter arity in findData's list-query normalizer (#7321) - #7386
Conversation
…t-query normalizer (#7321) `IHttpRequest.query` is `Record< string, string | string[] >` and the array arm is produced by a real first-party adapter, but every coercion in the shared list-query normalizer was written for the string arm. `Number(['1','2'])` is `NaN`, so `?$top=1&$top=2` reached the driver as `limit: NaN`; the same survey found the shape again on `$skip`/`offset`, `$search`, `$count`, the POST body's `object` copy, `having`, and the leftover-key bucket (`?status=open&status=won` lowered to a bare-array field spec that matches no row on any backend). Refused with 400 / INVALID_REQUEST — the code this normalizer already answers for the identical condition reached the other way (#4181 / #3795). A one-element array is one occurrence and is unwrapped; an empty array is no occurrence. The judgement is per parameter, not a sweep: `fields`/`select`/`$select`, `expand`/`populate`/`$expand`, `searchFields`, `orderBy`/`sort`/`$orderby`, `where`/`filter`/`filters`/`$filter` (whose array arm is a FILTER AST), `groupBy`, `aggregations` and the retired array keys all keep their array arm. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
ACCEPT — PM review (step 7), anchored to head
|
| file | status | matches declaration |
|---|---|---|
packages/metadata-protocol/src/protocol.ts |
modified (+187/−9) | yes — three hunks: the module constants after WIRE_QUERY_ALIAS_SLOTS, the assertQueryParamArity(options) call site in findData, and the $-alias loop rewritten to read the hoisted WIRE_DOLLAR_ALIASES |
packages/metadata-protocol/src/protocol.query-param-arity.test.ts |
added | yes — 46 cases in three labelled blocks |
.changeset/findata-query-param-arity.md |
added | yes |
All three STOP conditions clear, and the region fence held. packages/spec untouched — the disposition table reads existing declarations from query.zod.ts rather than changing what is accepted, which is the distinction that would have routed it to #6017. packages/rest untouched at zero bytes with no new dependency edge. None of #7306's six regions entered: the contributor discriminator and D9.9 bindings, both hydration seams, saveMetaItem, deleteMetaItem's layer subtraction and installedPackageBindingForObject are all outside this diff, which lives in the query-alias tables and findData. No content/docs/releases/, no docs/adr/**.
CI, per job
25 check runs, all completed: 23 success, 2 skipped (Build Docs, Console Pin Gate — path-filtered, correct for this diff). No failure, nothing pending. Named gates: ESLint, TypeScript Type Check, Build Core, Test Core 3/3 + rollup, Dogfood Regression Gate 3/3 + rollup, Dogfood Verify CLI, Temporal Conformance (live PG + MySQL), Check Changeset, Check PR Size, Console Pin Freshness, No other open PR may claim the same issue, ADR maintainer approval — every one success.
What I am accepting
The survey was delivered, and its yield is worse than the card's headline. The card was filed on one measured line (Number(['1','2']) is NaN, so ?$top=1&$top=2 reaches the driver as limit: NaN). The survey found the leftover-key bucket is the more damaging half: ?status=open&status=won lowers to where: { status: ['open','won'] }, and packages/formula/src/matches-filter.ts:187 is a bare if (Array.isArray(spec)) return false — so every row fails to match and the caller gets an empty page under a 200, which is the #4134 shape. A wrong-but-loud NaN is a smaller problem than a silently empty result set. Also surfaced, and named by neither the card nor my dispatch: Number([]) is 0, so an empty array on limit used to become limit: 0.
The judgement keys off the declared type rather than the request, and the reasoning for that is the part I most wanted to see. This normalizer serves both GET /data/:object (repeated querystring) and POST /data/:object/query (JSON body) and genuinely cannot tell them apart — so Array.isArray at this boundary is ambiguous as a signal about the request, and unambiguous only relative to what the slot declares. On a slot that never declares an array it is evidence of repetition; on one that does, it is the ordinary shape. That is why the disposition table is derived from the same alias tables the fold uses, so a newly added alias inherits its array arm instead of silently starting to 400.
Ordering ahead of the $-alias pass and the #3795 fold is load-bearing, not stylistic, and the PR proves it rather than asserting it: on the unfixed base, ?top=1&top=2&limit=1 is refused as Conflicting query parameters: 'limit', 'top' are spellings of the same parameter — a true refusal with a false diagnosis. Running arity first means the caller is told what is actually wrong, and the message quotes the spelling they wrote (#4226).
The dependency direction was answered out loud instead of being quietly reimplemented, which is exactly what I asked for. #6307's readSingleQueryValue lives in packages/rest/src/query-multiplicity.ts; @objectstack/metadata-protocol does not depend on @objectstack/rest and that edge would run backwards, so the shape was reused and not the module — with one deliberate divergence that has a measured reason: length-0 is deleted rather than set to undefined, because the leftover bucket reads Object.keys(options) and a key left carrying undefined would be lowered into an implicit { field: undefined } predicate. "Not supplied" has to mean absent here.
The rejection-vs-coercion answer is consistent in the way that matters. A thrown typed error at 400 / INVALID_REQUEST, chosen because conflictingQueryParamsError in this same normalizer already answers exactly that for the identical condition reached the other way (two spellings of one slot given different values, #4181 → #3795). One slot given two values must not carry two codes depending on how the caller got there. The cross-layer point is handled better than "make all three strings identical": rest spells its 400 VALIDATION_ERROR and runtime spells it VALIDATION_FAILED because those are each package's registered house catalog member, and what has to agree across the three layers is the rule and the status, which do.
Reverse verification — the four categories, and the variant is the reason this is accepted
Probe A (fix removed via git checkout <base SHA> -- protocol.ts, base pinned to the dev's own f188ed60e, not a moving origin/main; no git stash): predicted 18 red, measured 18 failed | 28 passed. The reds came in four distinct shapes, which is the justification for asserting code and status rather than toThrow() — most were silent acceptance ({"$top":["1","2"]} was ACCEPTED … instead of refused), but the object case already throws on the unfixed base with the wrong envelope (expected 'QUERY_OBJECT_MISMATCH' to be 'INVALID_REQUEST'), where a throw-only assertion would have been permanently green.
Missed predictions: none, each red for its predicted reason. Left unmeasured: none — vitest ran every case, nothing short-circuited.
Green in both directions, declared as guards not evidence: 28, and they are labelled [GUARD — green in both directions] in the file itself, not merely in the report.
Probe B, the variant, is what makes the guards mean something. Emptying ARRAY_VALUED_QUERY_SLOTS — i.e. replacing the per-parameter judgement with a blanket "no parameter may repeat" — was predicted to turn block 2 fully red while block 1 stayed fully green. Measured 24 failed | 22 passed, exactly as declared, with the most expensive red being The '$filter' query parameter was supplied 3 times. — the blanket rule reading the single filter ['status','=','open'] as three repetitions. Because block 1 stayed green under the variant, "refuse the scalars" and "preserve the lists" are two independent facts, and only the disposition table delivers both. That establishes the table as necessary, not merely sufficient, which is the standard this card was dispatched against.
Type-check debt was measured rather than assumed: 63 errors matching the existing DEBT entry, after the dev noticed their own first draft added a 64th (a TS2345 from the it.each tables) and typed it away. No new debt shipped.
Not folded in — correctly
#7390 is filed rather than fixed: a repeated ?filter= cannot be told from a filter AST at this layer (both are string[]), so it is refused as INVALID_FILTER "malformed filter array" — a true refusal naming the wrong cause — and in the narrow case where the repetition happens to spell a valid AST (?filter=status&filter=%3D&filter=open) it silently succeeds as {status:'open'}. Closing that needs either a packages/spec acceptance-surface change (#6017, not #6298) or a new ingress-provenance contract between packages/rest and packages/metadata-protocol. Both are decisions, neither was in this card's mandate, and folding either in would have made this PR's reverse verification attributable to more than one change. Filed unassigned as finding without pm:queue, which is the right posture for something that still needs grading.
Ruling
ACCEPT. Flipping out of draft and enabling auto-merge (SQUASH). Landing stays serial behind PR #7306 through the merge queue, as the region-disjoint parallel-authoring authorization requires.
Generated by Claude Code
2 participants
Fixes #7321
findData's shared list-query normalizer coerces query parameters without ever checking the arity it was handed. The work on this card is not that one line — it is the per-parameter survey around it. The same normalizer also folds four spellings of the filter slot, a large alias table, the$-alias consumption pass, and the leftover-key bucket that lowers unknown keys into field-equality predicates. Each needed the single-vs-multi judgement #6877 made for the REST layer, and several of those parameters accept the array arm on purpose.The measured fact, and the survey around it
IHttpRequest.queryis declaredRecord< string, string | string[] >, and the array arm is produced by a real first-party adapter (NodeHttpServerhands?x=1&x=2through as['1','2'], measured over a socket on #6878). Every coercion in this normalizer was written for the string arm:?$top=1&$top=2Number(['1','2'])→limit: NaN?$skip=1&$skip=2offset: NaN?status=open&status=wonwhere: { status: ['open','won'] }?$search=a&$search=b?$count=true&$count=falsetruenorfalse; left as-isobject: ['invoice','account']QUERY_OBJECT_MISMATCHThe leftover-key row is what the survey actually bought, and it is not a restatement of
limit: a bare array in{ status: ['open','won'] }is not an$in.packages/formula/src/matches-filter.tsline 187 isif (Array.isArray(spec)) return false;, so no row matches — precisely the #4134 failure shape (a filter that can never match answering 200 +total: 0).The disposition table — this is the deliverable
The judgement keys off the declared value type, not off the request. This normalizer serves both
GET /data/:object(a repeated querystring) andPOST /data/:object/query(a JSON body), and at this layer the two are indistinguishable — both are simplyArray.isArray. So the rule has to hang on the spec's declaration: on a slot that never declares an array, an array is unambiguous evidence of repetition; on a slot that does, it is the ordinary shape and must not be touched.Single-valued — repetition refused (new)
$top/top/limitz.number()$skip/skip/offsetz.number()$search/searchstring | FullTextSearch$count/countobjectz.string()havingFilterConditionobjectcursor/distinctLegitimately multi-valued — untouched by this PR
fields/select/$selectz.array(FieldNodeSchema)?$select=a&$select=bis the projection['a','b']orderBy/sort/$orderbyz.array(SortNodeSchema)normalizeSortNodeshas had astring[]arm since #4226; repetition composes into a multi-key sort rather than conflictingexpand/populate/$expand{name:{object:name}}searchFields/$searchFieldsz.array(z.string())where/filter/filters/$filter['status','=','open']is ONE filter, not three repetitionsgroupBy/aggregationsz.array(...)joins/windowFunctionsThe one that gets no arity check —
where— still refuses a repeated request, one block further down:?filter=A&filter=Barrives as['A','B'], whichisFilterASTcannot read. The diagnosis is less precise than the new message, but it is the honest answer available at this layer: an AST body form and a repeated querystring are byte-identical here.The rejection is a thrown typed error, not a coercion to the first value
GET /api/v1/notifications?limit=abccoerces to NaN and flows through listInbox's clamp intodata.find({ limit: NaN })— unvalidated query coercion on the notifications route #6928 / PR fix(runtime): refuse malformed notifications query params with an ADR-0112 400 (#6928) #7299 (GET /api/v1/notifications, whereNaNsurvived the clamp intodata.find({ limit: NaN })) and withGET/DELETE /packages/:id把重复的?version=查询参数(string[])原样交给 PackageService #6307 /packages/rest的其它req.query.*读取点同样把string | string[]当字符串用(#6307 的未扩大部分) #6877 (readSingleQueryValue). The rule is also the same: it counts occurrences, it does not compare values — two identical values are still two occurrences and are still refused; a one-element array is one occurrence and is unwrapped; an empty array is none.INVALID_REQUEST/ 400 is whatconflictingQueryParamsErrorin this same normalizer already answers for the identical condition reached the other way — two spellings of one slot carrying different values (REST 列表：无法解析的filterJSON 被静默忽略 —— 返回未过滤整页（#4134/#4164 家族第三员） #4181 → [P2]protocol.tsimplements 4 of the 5 documented RPC alias precedences backwards — and disagrees withhttp-dispatcher.tson three of them #3795). One slot given two values is one defect; it must not carry two codes depending on whether the caller repeatedfilteror wrotefilterandwhere.VALIDATION_ERRORand the runtime layerVALIDATION_FAILED, each being that package's registered 400 catalog member inerror-code-ledger.zod.ts. What has to agree across the three is the rule and the status, and it does. The message wording ispackages/rest的其它req.query.*读取点同样把string | string[]当字符串用(#6307 的未扩大部分) #6877'srepeatedQueryParamMessageverbatim.Dependency direction, stated rather than silently worked around. #6307's
readSingleQueryValuenow lives atpackages/rest/src/query-multiplicity.ts.@objectstack/metadata-protocoldoes not depend on@objectstack/rest, and that edge would be backwards — rest is the transport layer above this one. So the shape is reused and not the module. The one deliberate difference is also justified: the rest helper writes a zero-length array back asundefined, whereas here it must bedeleted, because the leftover bucket below readsObject.keys(options)and a key left behind carryingundefinedwould be lowered into a{field: undefined}predicate.Real file surface, file by file
packages/metadata-protocol/src/protocol.tsWIRE_DOLLAR_ALIASES,ARRAY_VALUED_QUERY_SLOTS,ARRAY_VALUED_LIST_QUERY_PARAMS,repeatedQueryParamError,assertQueryParamArity, placed afterWIRE_QUERY_ALIAS_SLOTS; (2)findDatacallsassertQueryParamArity(options)afterdelete options.contextand before the$-alias pass; (3) the$-alias loop reads the hoisted table instead of its own inline literal.packages/metadata-protocol/src/protocol.query-param-arity.test.ts.changeset/findata-query-param-arity.md@objectstack/metadata-protocol.Region fence (#7306). This PR enters none of #7306's regions in this same file: the D9.8/D9.9 contributor discriminator and bindings, both hydration seams,
saveMetaItem's producer-side refusal, the delete heal's layer subtraction indeleteMetaItem, and the retiredinstalledPackageBindingForObjectare all untouched.Reverse verification — four categories, direction declared before running
Pinned to my own base SHA,
f188ed60e697e970422a6c5ab414f8eb9e68c789, not a movingorigin/main.Probe A — take the fix out back to base (
git checkout BASE_SHA -- protocol.ts, keeping the test file)Declared beforehand: block 1 (refusal) all red; block 2 (preservation of the legitimately-multi parameters) all green, because those are guards; in block 3,
{status:['open']}and the empty-array case red,{$top:['5']}green.Measured:
Tests 18 failed | 28 passed (46)— matching the prediction case by case.objectcase throws on the unfixed base, with the wrong envelope — atoThrow()-only assertion would have been permanently green here:?top=1&top=2&limit=1is the other "throws, but the diagnosis is false" case. On base:Number([])is0, so on baselimit: []becamelimit: +0andstatus: []becamewhere: {status: []}:{$top:['5']}(Number(['5'])is already 5, since a one-element array stringifies to its element). All three groups carry a[GUARD — green in both directions]marker in the test file.Probe B — the variant: replace the per-parameter judgement with a blanket arity rejection
This is the damage this card is most likely to cause, so it is measured rather than argued. Method: empty
ARRAY_VALUED_QUERY_SLOTS(= []), which is exactly "any array of length greater than 1 is a 400".Declared beforehand: block 1 stays fully green (the refusals still fire); block 2 goes fully red; the comma-list projection
?$select=name,statusstays green because it is a string, not an array; block 3 stays green because one-element and empty arrays are accepted under both rules.Measured:
Tests 24 failed | 22 passed (46)— all of it as declared. The 24 reds are exactly the preservation cases:The most expensive one is the filter AST:
['status','=','open']is one filter, and the blanket rule reads it as three repetitions. That is the proof the disposition table is necessary and not merely sufficient: block 1 stayed entirely green under the variant, so the "refuse" half and the "preserve" half are two independent facts, and only the table delivers both.Tests
STOP conditions — none of the three tripped
packages/specacceptance surface: untouched. The disposition table reads declarations that already exist (query.zod.ts'sz.number()/z.array(...)); nothing changes what the schema accepts or carries.packages/restbehaviour: not one byte. Nometadata-protocoltorestdependency was introduced (the direction is inverted), and no rest-side behaviour changed.Generated by Claude Code