Skip to content

[P2] protocol.ts implements 4 of the 5 documented RPC alias precedences backwards — and disagrees with http-dispatcher.ts on three of them #3795

Description

@os-zhuang

Found by the alias sweep #3754 called for ("the remaining @deprecated/alias declarations in packages/spec/src deserve the same audit — the ones that fold-and-drop are safe, the ones that merely coexist are latent #3713s"). This is the sweep's result: one cluster is not merely latent, it is live and inverted.

The contract

RpcQueryOptionsSchema accepts five legacy aliases alongside their canonical keys, and states the precedence in prose only — there is no transform folding them:

// packages/spec/src/data/data-engine.zod.ts (RpcQueryOptionsSchema doc block)
 * **Precedence:** When both legacy and new keys are present for the same
 * concern, the protocol normalizer uses the new key (`where` > `filter`,
 * `fields` > `select`, `orderBy` > `sort`, `offset` > `skip`,
 * `expand` > `populate`). Callers should not mix vocabularies.

Because the fold lives nowhere in the schema, every reader re-implements it — the exact condition #3713 described. There are two readers, and they do not agree.

What the two normalizers actually do

pair spec says runtime/src/http-dispatcher.ts metadata-protocol/src/protocol.ts
where > filter/filters canonical where ?? filter ?? filters filter ?? filters ?? $filter ?? where
fields > select canonical select != null && fields == null select clobbers fields
offset > skip canonical skip != null && offset == null skip clobbers offset
expand > populate canonical populate wins, expand only if empty
orderBy > sort canonical orderBy ?? sort

Four of five inverted in protocol.ts; three of them are pairs where the two normalizers do the opposite thing.

The four inversions

All in packages/metadata-protocol/src/protocol.ts. Canonical where is consulted last:

const filterValue = options.filter ?? options.filters ?? options.$filter ?? options.where;

skip overwrites a caller's offset unconditionally:

if (options.skip != null) {
    options.offset = Number(options.skip);   // no `offset == null` guard
    delete options.skip;
}

select overwrites fields unconditionally:

if (typeof options.select === 'string') {
    options.fields = options.select.split(',');   // no `fields == null` guard
} else if (Array.isArray(options.select)) {
    options.fields = options.select;
}

The deprecated populate is consulted first, canonical expand only when populate produced nothing:

const populateValue = options.populate;
const expandValue = options.$expand ?? options.expand;
const expandNames: string[] = [];
if (typeof populateValue === 'string')      expandNames.push();
else if (Array.isArray(populateValue))      expandNames.push();
if (!expandNames.length && expandValue)     {  }   // canonical is the fallback

Why this reads as an oversight, not a decision

The same function, a few lines above the skip block, folds the OData $-prefixed params with the correct guard:

if (options[dollar] != null && options[bare] == null) {
    options[bare] = options[dollar];
}

So the canonical-wins pattern was known and applied there; the legacy pairs never got it.

Impact

Three pairs are handled by both normalizers with opposite precedence, so the same request resolves differently depending on which path serves it:

  • ?filter=X&where=YY through the dispatcher, X through the metadata protocol.
  • ?select=a&fields=b[b] vs [a].
  • ?skip=10&offset=00 vs 10.

Nothing drops these keys at parse (unlike action.execute after #3742 or field.conditionalRequired after #3764), so both keys reach every consumer and the divergence is reachable from a plain HTTP request. The spec anticipates mixing explicitly — it documents what happens when callers do — so "callers should not mix vocabularies" is guidance, not a guarantee.

Suggested fix

The shape #3742 / #3764 established, applied one layer down:

  1. Add a transform to RpcQueryOptionsSchema that folds each legacy alias into its canonical key when the canonical is absent, and drops the alias from the parsed output. Canonical wins when both are set — matching what the doc block already promises.
  2. With the aliases gone from parsed options, delete the hand-rolled folds in both normalizers rather than fixing them in place — the point is to decide precedence once, not to make N readers agree by inspection.
  3. Where a normalizer must still accept raw (unparsed) input, keep a single fold helper shared by both packages instead of two open-coded copies.
  4. Pin it: a test asserting each alias key is absent from parsed output, and one per pair asserting canonical wins.

Note this is a minor for @objectstack/spec on the same reasoning as #3742 / #3764 — parsed output loses deprecated keys, so a TS consumer reading parsed.populate fails to compile instead of silently reading undefined.

Scope note

The visibleWhen family (visibleOn / visibility) is already correctnormalizeVisibleWhen destructures the aliases out and returns the rest, i.e. fold-and-drop. It was checked as part of this sweep and needs no change. The remaining @deprecated markers in packages/spec/src are renamed schema/type exports (theme.zod.ts, stack.zod.ts, the Engine*OptionsSchema aliases), which carry no object-key precedence risk.

$filter / $expand / $top (OData spellings) are a separate compatibility layer and are left as-is here; note only that $expand ?? expand also puts the OData spelling ahead of the canonical key, which is worth settling in the same pass.

Refs #3713, #3742, #3754, #3764.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions