From 3a683ead5e3b6db21289b193e41de570a869d6bf Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 02:29:13 +0000 Subject: [PATCH] fix(app-shell,plugin-dashboard,console): send query options with their `$`, and gate the shape (#5458) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `QueryParams` declares every query option `$`-prefixed and `convertQueryParams` copies exactly those keys, so an unprefixed spelling reaches no branch and is dropped — silently, and it type-checks because the type carries `[key: string]: any` for adapter-specific params. A dropped cap makes the read UNBOUNDED rather than truncated: the platform's GET list route has no default page size. Four live sites, all fixed: - `ObjectView` fetched the footer record count with `{ limit: 0 }` — the one that INVERTED, since `$top: 0` means "no records": "count only" became "fetch every row", on every mount of every list view. Now `$top: 0`, reading `total` only; the row-counting fallbacks are dropped rather than repointed, because an empty `data` after asking for zero rows says nothing about the object. - `AssignedUsersSection` — `{ …, limit: 1 }`, one line from three correct `$top` calls. - `DashboardFilterBar` — `fields` AND `top` in one literal, so a filter's option list read every row and every column of its source; it also read `records.items`, not a `QueryResult` member, so a real adapter yielded no options at all. Found by the new rule, not by the card. - `sdui-workbench-preview` — `{ top: 200 }` plus a `.records` misread, inside page-source metadata. New `object-ui/no-unprefixed-query-params` rejects the shape at write time: a known query-option name missing its `$` in the second argument of a `find`/`findOne` call. Narrow on purpose — a closed spelling list anchored to the call — because the index signature exists for adapter-specific params. The sibling `no-query-params-under-options` is unchanged and still gates its half. Part of #5458 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PQ3NihCHE9LUtHoGxo6A9f --- .changeset/5458-unprefixed-query-params.md | 39 +++ apps/console/src/sdui-workbench-preview.tsx | 4 +- eslint-rules/index.js | 2 + eslint-rules/no-unprefixed-query-params.js | 221 +++++++++++++++ .../no-unprefixed-query-params.test.js | 265 ++++++++++++++++++ eslint.config.js | 17 ++ packages/app-shell/src/views/ObjectView.tsx | 27 +- .../metadata-admin/AssignedUsersSection.tsx | 2 +- .../src/DashboardFilterBar.tsx | 20 +- .../DashboardFilterBar.options.test.tsx | 60 +++- 10 files changed, 642 insertions(+), 15 deletions(-) create mode 100644 .changeset/5458-unprefixed-query-params.md create mode 100644 eslint-rules/no-unprefixed-query-params.js create mode 100644 eslint-rules/no-unprefixed-query-params.test.js diff --git a/.changeset/5458-unprefixed-query-params.md b/.changeset/5458-unprefixed-query-params.md new file mode 100644 index 0000000000..040a74bb72 --- /dev/null +++ b/.changeset/5458-unprefixed-query-params.md @@ -0,0 +1,39 @@ +--- +'@object-ui/app-shell': patch +'@object-ui/plugin-dashboard': patch +'@object-ui/console': patch +--- + +Fix four `find()` calls that passed a query option without its `$`, and gate the shape. + +`QueryParams` declares every query option `$`-prefixed and `convertQueryParams` copies +exactly those keys, so an unprefixed spelling reaches no branch and is dropped — no throw, +no warning, and it type-checks because the type carries `[key: string]: any` for +adapter-specific params. For a dropped cap the result is an **unbounded** read rather than +a truncated one: the platform's GET list route has no default page size, so the query +returns the whole match set and stays invisible until the object is large. + +- `app-shell` `ObjectView` fetched the footer's record count with `{ limit: 0 }`. This one + **inverted** rather than widened — `$top: 0` is honoured end to end as "no records", so + the dropped key turned "count only, fetch nothing" into "fetch every row in the object", + on every mount and every refresh of every list view. It now sends `$top: 0` and reads + the count off `total` only; the row-counting fallbacks are gone rather than repointed, + because once zero rows are requested an empty `data` means "you asked for none", not + "the object is empty", and counting it would assert a confident `0`. With no total the + footer line is omitted instead. +- `app-shell` `AssignedUsersSection` looked a permission set up with `{ …, limit: 1 }`, + one line from three correct `$top` calls. +- `plugin-dashboard` `DashboardFilterBar` passed `fields` **and** `top` in one literal, so + a filter's option list read every row and every column of its source object while its + own comment described it as capped at 200. The same call read `records.items`, which is + not a `QueryResult` member, so against a real adapter the fallback produced no options + at all. +- `console` `sdui-workbench-preview` passed `{ top: 200 }` and read `.records` off the + result in its page-source metadata. + +A new `object-ui/no-unprefixed-query-params` ESLint rule rejects the shape at write time: +a known query-option name missing its `$` in the second argument of a `find`/`findOne` +call. It is narrow on purpose — a closed list of spellings, anchored to the call — because +the index signature exists so adapters can take adapter-specific params, and a rule that +flagged any unprefixed key would report the shape the type was written to allow. Its +sibling `no-query-params-under-options` (the `{ options: { $top } }` half) is unchanged. diff --git a/apps/console/src/sdui-workbench-preview.tsx b/apps/console/src/sdui-workbench-preview.tsx index f4d5de9c45..ef984a19c1 100644 --- a/apps/console/src/sdui-workbench-preview.tsx +++ b/apps/console/src/sdui-workbench-preview.tsx @@ -83,8 +83,8 @@ function Page() { const [stats, setStats] = React.useState({ total: 0, active: 0 }); const refreshStats = React.useCallback(async () => { if (!adapter) return; - const all = await adapter.find('showcase_project', { top: 200 }); - const rows = Array.isArray(all) ? all : (all && all.records) || []; + const all = await adapter.find('showcase_project', { $top: 200 }); + const rows = Array.isArray(all) ? all : (all && all.data) || []; setStats({ total: rows.length, active: rows.filter((r) => r.status === 'active').length }); }, [adapter]); React.useEffect(() => { refreshStats(); }, [refreshStats, reloadKey]); diff --git a/eslint-rules/index.js b/eslint-rules/index.js index 3f84432fbe..2d3fa8159d 100644 --- a/eslint-rules/index.js +++ b/eslint-rules/index.js @@ -6,6 +6,7 @@ import noInlineSpecConfig from './no-inline-spec-config.js'; import noTryCatchAroundHook from './no-try-catch-around-hook.js'; import noDynamicImportInTestHook from './no-dynamic-import-in-test-hook.js'; import noQueryParamsUnderOptions from './no-query-params-under-options.js'; +import noUnprefixedQueryParams from './no-unprefixed-query-params.js'; import buttonHasType from './button-has-type.js'; import noUnpairedBadgeColorClasses from './no-unpaired-badge-color-classes.js'; @@ -16,6 +17,7 @@ export default { 'no-try-catch-around-hook': noTryCatchAroundHook, 'no-dynamic-import-in-test-hook': noDynamicImportInTestHook, 'no-query-params-under-options': noQueryParamsUnderOptions, + 'no-unprefixed-query-params': noUnprefixedQueryParams, 'button-has-type': buttonHasType, 'no-unpaired-badge-color-classes': noUnpairedBadgeColorClasses, }, diff --git a/eslint-rules/no-unprefixed-query-params.js b/eslint-rules/no-unprefixed-query-params.js new file mode 100644 index 0000000000..4e76f41762 --- /dev/null +++ b/eslint-rules/no-unprefixed-query-params.js @@ -0,0 +1,221 @@ +/** + * ObjectUI ESLint rule: no-unprefixed-query-params + * + * Sibling of `no-query-params-under-options`, for the other half of the same + * defect class: a query option written WITHOUT its `$`, at the TOP level of a + * `find`/`findOne` params object — `find(obj, { top: 200 })` instead of + * `find(obj, { $top: 200 })`. + * + * `QueryParams` (`packages/types/src/data.ts`) declares every query option with + * a leading `$` — `$select`, `$filter`, `$orderby`, `$skip`, `$top`, `$expand`, + * `$search`, `$searchFields`, `$count` — and `convertQueryParams` in + * `@object-ui/data-objectstack` builds its outgoing options by copying exactly + * those. An unprefixed key reaches no branch and is dropped: no throw, no + * warning. `QueryParams` also carries `[key: string]: any`, so the type system + * accepts both spellings equally and nothing rejects the dead one. + * + * The consequence for a dropped cap is an UNBOUNDED read, not a truncated one. + * The platform's GET list route has no default page size — the pinned + * `@objectstack/client` serializes `top` only when the caller supplied it + * (`if (normalizedOptions.top != null)`) — so an absent `top` returns the whole + * match set. The symptom is therefore invisible until the object is large, and + * when it appears it reads as a data problem rather than a code problem. + * + * Three live sites existed when this rule landed (objectui#5458): + * + * - `app-shell` `ObjectView.tsx` — `find(name, { limit: 0 })` for the footer's + * record count. `$top: 0` is honoured end to end as "no records", so the + * dropped key did not merely widen the read, it INVERTED the call: "count + * only, fetch nothing" became "fetch every row in the object", on every + * mount and every refresh of every list view. + * - `app-shell` `metadata-admin/AssignedUsersSection.tsx` — `{ $filter: {…}, + * limit: 1 }`, one line away from three correct calls (`$top: 500`, `$top: + * 200`, `$top: 1000`). Half-correct spelling in a single object literal is + * the shape a reviewer's eye slides over, which is the argument for + * mechanising this rather than reviewing it. + * - `apps/console` `sdui-workbench-preview.tsx` — `{ top: 200 }`, inside a + * template literal holding runtime page metadata. See the boundary note + * below: this rule does NOT catch that one, and cannot. + * + * A FOURTH site, which the card that commissioned this rule never named, was + * found by the rule itself on its first repo-wide run: `plugin-dashboard` + * `DashboardFilterBar.tsx` passed `fields` AND `top` in the same literal, so a + * dashboard filter's option list read every row and every column of its source + * object while its own comment described it as capped at 200. That is the + * argument for the rule in one site: two dropped keys, in one call, under a + * comment asserting the opposite. + * + * ## Scope, deliberately narrow so it discriminates + * + * Two independent narrowings, both load-bearing: + * + * 1. **Only a KNOWN query-option name.** Not "any unprefixed key". The index + * signature exists because adapters legitimately take adapter-specific + * params, so flagging every unprefixed key would report the shape the type + * was written to allow — and a rule that cries wolf gets switched off. The + * list below is closed and every entry maps to a real `QueryParams` key. + * 2. **Only the second argument of a `find`/`findOne` CALL.** Unlike its + * sibling — whose `options`-holding-a-`$`-key signature is unmistakable + * anywhere — every name on the list (`top`, `limit`, `filter`, `sort`, + * `select`, `count`, …) is a perfectly ordinary object key elsewhere in + * this repo. Outside a finder call the name carries no signal at all, so + * the call is what makes the report meaningful. + * + * `Array.prototype.find` is a `.find(` member call too. It is excluded by its + * own signature rather than by naming: its first argument is a predicate, so a + * call whose first argument is a function literal is skipped. The key list is a + * second, independent filter — an array `find`'s optional second argument is a + * `thisArg`, which is not an object literal carrying `top`/`limit`. + * + * Only STATIC key names report — an identifier or a string literal. A computed + * key (`{ [k]: v }`) cannot be judged without running the code and a spread + * (`{ ...params }`) hides its keys, so neither says anything. Same rule as the + * sibling, same reason: null always means "say nothing". + * + * ## Known boundaries, recorded rather than silently omitted + * + * - **Source inside a string is invisible to it.** `sdui-workbench-preview.tsx` + * holds its page source in a template literal, which the parser sees as one + * `TemplateLiteral` token and never as a `CallExpression`. No AST rule can + * reach it. That site was fixed by hand for objectui#5458; a text scan is + * the only mechanism that would gate it, and a text scan over this key list + * would match the prose in this very file. + * - **Only these spellings.** `orderBy` (camelCase) and `pageSize` are the + * same mistake in spirit and are deliberately NOT listed: no site has ever + * used them here, and the argument for this rule is that it discriminates. + * Add one when a real instance appears, with the instance cited — which is + * exactly how `fields` got on the list. It is not a `$`-less `$fields` (no + * such key exists); it is an alias for `$select`, the same kind of entry as + * `limit` -> `$top` and `offset` -> `$skip`, and it earned its place on a + * live site: `plugin-dashboard` `DashboardFilterBar.tsx` passed `fields` and + * `top` in ONE literal, so the filter-option query fetched every row and + * every column of the source object. + * + * No autofixer. `limit` and `offset` do not rename to `$limit`/`$offset` — they + * rename to `$top`/`$skip` — and `filters` collapses to `$filter`, so a fix + * would have to merge into a key that may already be present in the same + * literal. More importantly `ObjectView`'s `limit: 0` needed a judgement about + * what the call MEANT before it could be rewritten; a fixer would have made + * that silently. objectui#5458. + * + * @type {import('eslint').Rule.RuleModule} + */ + +/** + * Unprefixed spellings that are really query options, mapped to the + * `QueryParams` key each one means. Closed on purpose — see the scope note. + */ +const QUERY_OPTION_SPELLINGS = { + top: '$top', + limit: '$top', + skip: '$skip', + offset: '$skip', + filter: '$filter', + filters: '$filter', + select: '$select', + fields: '$select', + orderby: '$orderby', + sort: '$orderby', + expand: '$expand', + search: '$search', + count: '$count', +}; + +/** Methods whose second argument is a `QueryParams`. */ +const FINDER_METHODS = new Set(['find', 'findOne']); + +/** + * Static name of a property key, or null when it cannot be read off the AST + * (computed keys, spreads). Null always means "say nothing". + */ +function staticKeyName(node) { + if (node.type === 'SpreadElement' || node.type === 'ExperimentalSpreadProperty') return null; + if (node.computed) return null; + const key = node.key; + if (key.type === 'Identifier') return key.name; + if (key.type === 'Literal' && typeof key.value === 'string') return key.value; + return null; +} + +/** + * Static method name of a member call (`a.find`, `a?.find`, `a['find']`), or + * null for anything whose callee cannot be read off the AST. + */ +function calleeMethodName(callee) { + if (!callee || callee.type !== 'MemberExpression') return null; + const property = callee.property; + if (callee.computed) { + return property.type === 'Literal' && typeof property.value === 'string' + ? property.value + : null; + } + return property.type === 'Identifier' ? property.name : null; +} + +/** + * Look through the TypeScript wrappers that do not change the value, so a + * params literal written `{ limit: 1 } as QueryParams` is still read as the + * object literal it is. The index signature makes that cast compile, which is + * exactly the population this rule exists for — an evasion by `as` would be + * silent and would look deliberate. + */ +function unwrapExpression(node) { + let current = node; + while ( + current + && (current.type === 'TSAsExpression' + || current.type === 'TSSatisfiesExpression' + || current.type === 'TSNonNullExpression' + || current.type === 'TSTypeAssertion') + ) { + current = current.expression; + } + return current; +} + +/** `arr.find(predicate)` — an array search, not a data read. */ +function firstArgumentIsPredicate(node) { + const first = unwrapExpression(node.arguments[0]); + if (!first) return false; + return first.type === 'ArrowFunctionExpression' || first.type === 'FunctionExpression'; +} + +export default { + meta: { + type: 'problem', + docs: { + description: + 'Disallow a query option spelled without its `$` in the params of a `find`/`findOne` call — `QueryParams` declares them all `$`-prefixed and `convertQueryParams` copies exactly those, so the unprefixed key is dropped and the query silently runs unbounded (objectui#5458).', + recommended: true, + }, + schema: [], + messages: { + unprefixedQueryOption: + '`{{key}}` is not a `QueryParams` key — write `{{canonical}}`. `QueryParams` (@object-ui/types) declares every query option with a leading `$`, and `convertQueryParams` (@object-ui/data-objectstack) copies exactly those keys, so `{{key}}` reaches no branch and is dropped: no throw, no warning. Its `[key: string]: any` index signature exists for adapter-specific params, which is why the dead spelling type-checks. When the dropped key is a cap the read becomes UNBOUNDED rather than truncated — the platform GET list route has no default page size, so the query returns the whole match set and stays invisible until the object is large. See objectui#5458.', + }, + }, + create(context) { + return { + CallExpression(node) { + if (!FINDER_METHODS.has(calleeMethodName(node.callee))) return; + if (firstArgumentIsPredicate(node)) return; + + const params = unwrapExpression(node.arguments[1]); + if (!params || params.type !== 'ObjectExpression') return; + + for (const property of params.properties) { + const key = staticKeyName(property); + if (key === null) continue; + const canonical = QUERY_OPTION_SPELLINGS[key]; + if (!canonical) continue; + + context.report({ + node: property.key, + messageId: 'unprefixedQueryOption', + data: { key, canonical }, + }); + } + }, + }; + }, +}; diff --git a/eslint-rules/no-unprefixed-query-params.test.js b/eslint-rules/no-unprefixed-query-params.test.js new file mode 100644 index 0000000000..52466032b4 --- /dev/null +++ b/eslint-rules/no-unprefixed-query-params.test.js @@ -0,0 +1,265 @@ +/** + * Pins `no-unprefixed-query-params` in BOTH directions, because this rule is + * only worth having if it discriminates. Every name on its list — `top`, + * `limit`, `filter`, `sort`, `select`, `fields`, `count`, `offset` — is a + * perfectly ordinary object key in this repo, so unlike its sibling + * (`no-query-params-under-options`, whose `$`-key-under-`options` signature is + * unmistakable anywhere) this one carries no signal at all away from a finder + * call. A version that matched on the name alone would be uninstallable, and + * the valid cases below are what say so. + * + * The invalid cases are the four live sites this rule was written for, in the + * exact form they shipped (objectui#5458): + * + * - `app-shell` `ObjectView.tsx` — `find(name, { limit: 0 })`. The one that + * INVERTED rather than widened: `$top: 0` means "no records", so the + * dropped key turned "count only, fetch nothing" into "fetch every row", + * on every mount and every refresh of every list view. + * - `app-shell` `metadata-admin/AssignedUsersSection.tsx` — `{ $filter: {…}, + * limit: 1 }`, one line from three CORRECT calls. Those three neighbours + * are in the valid list below as the false-positive control: half-correct + * spelling inside one literal is the whole shape of this defect, so a rule + * that reported the correct half would be worse than none. + * - `plugin-dashboard` `DashboardFilterBar.tsx` — `fields` and `top` in ONE + * literal, under a comment describing the query as capped at 200 records. + * The card never named this site; the rule found it on its first repo-wide + * run, which is the argument for mechanising the family. + * - `apps/console` `sdui-workbench-preview.tsx` — deliberately ABSENT here, + * and that absence is the point. Its `find` call lives inside a template + * literal holding runtime page metadata, so the parser sees one + * `TemplateLiteral` and never a `CallExpression`. No AST rule can reach it; + * it was fixed by hand. Asserting it here would be asserting a capability + * the rule does not have. + */ +import { describe, it, afterAll } from 'vitest'; +import { RuleTester } from 'eslint'; +import tseslint from 'typescript-eslint'; +import rule from './no-unprefixed-query-params.js'; +import siblingRule from './no-query-params-under-options.js'; + +RuleTester.afterAll = afterAll; +RuleTester.it = it; +RuleTester.describe = describe; + +const ruleTester = new RuleTester(); + +ruleTester.run('no-unprefixed-query-params', rule, { + valid: [ + // ── The correct spelling — the whole point of the rule. + `dataSource.find(objectName, { $top: 100 });`, + `dataSource.find(objectName, { $filter: filter, $top: 100, $orderby: 'name asc' });`, + `adapter.findOne(objectName, { $select: ['id', 'name'] });`, + + // ── FALSE-POSITIVE CONTROL: the three correct neighbours that sit within a + // few lines of the `limit: 1` site in AssignedUsersSection.tsx, verbatim. + // The rule reported the fourth call in that file and none of these. + `adapter.find('sys_user_permission_set', { $filter: { permission_set_id: id }, $top: 500 });`, + `adapter.find('sys_position_permission_set', { $filter: { permission_set_id: id }, $top: 200 });`, + `adapter.find('sys_user', { $filter: { id: { $in: userIds } }, $top: 1000 });`, + + // ── `Array.prototype.find`, which is a `.find(` member call too. Excluded + // by its own signature: a predicate first argument. Real ObjectView shapes. + `const objectDef = objects.find((o) => o.name === objectName);`, + `const targetView = views.find(function (v) { return v.id === vid; });`, + // …including the two-argument form, where `thisArg` is the second argument. + `rows.find(function (r) { return r.id === id; }, { count: 0, limit: 1 });`, + + // ── The adapter-specific params the index signature exists for. Flagging + // these is what "any unprefixed key" would have done, and why it isn't + // what this rule does. + `adapter.find(objectName, { $top: 10, includeDeleted: true, tenant: 'acme' });`, + `adapter.find(objectName, { $top: 10, cacheKey: key, signal: controller.signal });`, + + // ── A listed name away from a finder call carries no signal whatsoever. + `const widget = { type: 'table', options: { sortBy: 'amount', limit: 5 } };`, + `const pagination = { limit: 20, offset: 40, count: true };`, + `renderList({ top: 200, filters: rules });`, + `fetchPage(url, { limit: 50 });`, + // A method that is not `find`/`findOne`. The boundary is deliberate. + `adapter.query(objectName, { top: 200 });`, + `adapter.aggregate(objectName, { limit: 10 });`, + + // ── Position matters: only the SECOND argument is a `QueryParams`. + `client.find({ limit: 10 });`, + `adapter.find(objectName, { $top: 1 }, { limit: 5, retries: 2 });`, + + // ── Only the TOP level of that argument. A field genuinely NAMED `limit` + // or `count`, filtered on, is normal data and must stay silent. + `adapter.find('plan', { $filter: { limit: 5 }, $top: 20 });`, + `adapter.find('usage', { $filter: { count: { $gt: 10 }, offset: 3 } });`, + + // ── Shapes the rule cannot judge, so it says nothing. + `adapter.find(objectName, { [key]: 200 });`, + // A computed key is silent even when its value happens to be a readable + // string literal. `staticKeyName` is character-for-character the sibling + // rule's, and the two must not fork: "computed means say nothing" is one + // contract stated in both files, and a knowable-subset exception in one + // copy only is the kind of silent divergence duplicated helpers die of. + // No site has ever written a query option this way. + `adapter.find(objectName, { ['top']: 200 });`, + `adapter.find(objectName, { ...params });`, + `adapter.find(objectName, params);`, + `adapter.find(objectName);`, + + // ── The SIBLING's shape. `{ options: { $top: 100 } }` carries no + // unprefixed key at the top level, so THIS rule is correctly silent — + // `no-query-params-under-options` is what reports it, and the pin that + // it still does is at the bottom of this file. + `dataSource.find(objectName, { options: { $top: 100 } });`, + ], + + invalid: [ + // ── Live site 1: ObjectView.tsx, the inverting one. + { + code: `dataSource.find(objectDef.name, { limit: 0 }).then(read);`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + // ── Live site 2: AssignedUsersSection.tsx, beside the three valid cases above. + { + code: `adapter.find('sys_permission_set', { $filter: { name: permissionSetName }, limit: 1 });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + // ── Live site 3: DashboardFilterBar.tsx — TWO dropped keys in one literal, + // so two reports. `fields` is the projection (`$select`), `top` the cap. + { + code: `dataSource.find(from.object, { fields: [from.valueField], $filter: f, top: 200 });`, + errors: [ + { messageId: 'unprefixedQueryOption', data: { key: 'fields', canonical: '$select' } }, + { messageId: 'unprefixedQueryOption', data: { key: 'top', canonical: '$top' } }, + ], + }, + + // ── Every listed spelling reports, and reports the key it really means — + // `limit`/`offset`/`filters`/`sort`/`fields` do NOT rename to `$limit` + // and friends, which is also why there is no autofixer. + { + code: `adapter.find(o, { top: 1, skip: 2, filter: f, filters: g });`, + errors: [ + { key: 'top', canonical: '$top' }, { key: 'skip', canonical: '$skip' }, + { key: 'filter', canonical: '$filter' }, { key: 'filters', canonical: '$filter' }, + ].map((data) => ({ messageId: 'unprefixedQueryOption', data })), + }, + { + code: `adapter.find(o, { select: s, fields: f, orderby: 'a', sort: 'b' });`, + errors: [ + { key: 'select', canonical: '$select' }, { key: 'fields', canonical: '$select' }, + { key: 'orderby', canonical: '$orderby' }, { key: 'sort', canonical: '$orderby' }, + ].map((data) => ({ messageId: 'unprefixedQueryOption', data })), + }, + { + code: `adapter.find(o, { expand: e, search: q, count: true, limit: 5, offset: 10 });`, + errors: [ + { key: 'expand', canonical: '$expand' }, { key: 'search', canonical: '$search' }, + { key: 'count', canonical: '$count' }, { key: 'limit', canonical: '$top' }, + { key: 'offset', canonical: '$skip' }, + ].map((data) => ({ messageId: 'unprefixedQueryOption', data })), + }, + + // ── `findOne` is in scope for the same reason `find` is. + { + code: `adapter.findOne('contact', { select: ['id'] });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'select', canonical: '$select' } }], + }, + + // ── Static key spellings the rule must still read. + { + code: `adapter.find(o, { 'limit': 10 });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + { + code: `adapter?.find(o, { limit: 10 });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + { + code: `this.dataSource.find(o, { top: 200 });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'top', canonical: '$top' } }], + }, + { + code: `adapter['findOne'](o, { limit: 1 });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + + // ── The doc page's old shape, which prose now warns against by name. + { + code: `adapter.find(o, { filters: [['status', '=', 'open']] });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'filters', canonical: '$filter' } }], + }, + + // ── A correct key beside a dropped one: only the dropped one reports. + { + code: `adapter.find(o, { $filter: f, $orderby: 'name asc', limit: 25 });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + ], +}); + +/** + * The same rule under the parser it actually runs with. `eslint.config.js` + * applies it to every `.ts`/`.tsx` file through typescript-eslint, and + * TypeScript adds two shapes espree cannot express — a cast, and a type + * declaration. They must go opposite ways. + */ +const tsRuleTester = new RuleTester({ + languageOptions: { parser: tseslint.parser }, +}); + +tsRuleTester.run('no-unprefixed-query-params (typescript)', rule, { + valid: [ + // A TYPE is not a value, and none of these is a call. + `interface Page { limit?: number; offset?: number }`, + `type Finder = (o: string, p: { top?: number }) => void;`, + `declare function find(o: string, p?: { limit?: number }): void;`, + // Correct spelling, typed. + `const params: QueryParams = { $top: 100 }; adapter.find(o, params);`, + `adapter.find(o, { $top: 100 } as QueryParams);`, + ], + invalid: [ + // The index signature is what makes all three of these compile — this is + // the population the rule exists for, so a cast must not be an escape. + { + code: `adapter.find(o, { limit: 1 } as QueryParams);`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + { + code: `adapter.find(o, { top: 200 } satisfies QueryParams);`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'top', canonical: '$top' } }], + }, + { + code: `dataSource!.find(o, { limit: 0 });`, + errors: [{ messageId: 'unprefixedQueryOption', data: { key: 'limit', canonical: '$top' } }], + }, + ], +}); + +/** + * REGRESSION CONTROL for the half that was already gated (objectui#4734). + * + * This rule is a SIBLING, not a replacement: the two anchor on different + * shapes, carry different messages, and must be separately silenceable — one + * `eslint-disable` may not switch off both halves of the class. Landing this + * file must therefore leave `{ options: { $top: 100 } }` failing exactly as it + * did before, so the sibling is re-run here on the two instances it was written + * for. Its own test file covers it in full; this is the cross-check that the + * two rules still divide the class between them and neither has swallowed the + * other. + */ +const siblingTester = new RuleTester(); + +siblingTester.run('no-query-params-under-options (still gated)', siblingRule, { + valid: [ + // The shape THIS card fixed is the new rule's business, not the sibling's. + `dataSource.find(objectName, { limit: 0 });`, + `dataSource.find(objectName, { $top: 100 });`, + ], + invalid: [ + // object-timeline (objectui#4009) and object-kanban (objectui#4025). + { + code: `find(objectName, { options: { $top: 100 } });`, + errors: [{ messageId: 'deadOptionsKey' }], + }, + { + code: `dataSource.find(objectName, { $filter: filter, options: { $top: 100 } });`, + errors: [{ messageId: 'deadOptionsKey' }], + }, + ], +}); diff --git a/eslint.config.js b/eslint.config.js index 9bacf99c68..8c83f0870e 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -118,6 +118,23 @@ export default tseslint.config({ // signature needs no exemptions — 47 legitimate `options` objects exist // repo-wide and not one carries a `$`-prefixed key. 'object-ui/no-query-params-under-options': 'error', + // objectui#5458 ratchet — the other half of the same class: + // `find(obj, { top: 200 })`, the query option spelled without its `$` at + // the TOP level. `convertQueryParams` copies exactly the `$`-prefixed keys + // `QueryParams` declares, so the bare spelling is dropped with no throw and + // no warning, and the same `[key: string]: any` makes it type-check. Three + // live sites, and the app-shell one INVERTED rather than widened: + // `find(name, { limit: 0 })` fetched the footer's record count by reading + // every row in the object, on every mount of every list view, because + // `$top: 0` is honoured end to end as "no records" and `limit` reached + // nothing. A sibling rule rather than a second predicate on the one above: + // that rule's signature (`$`-key under `options`) is unmistakable in any + // object literal, while every name on this one's list is an ordinary key + // outside a finder call, so the two need different anchors — and one + // `eslint-disable` must not silence both halves. Error so the next one + // fails at write time; all three sites were converted first, so this lints + // clean today with no allowlist. + 'object-ui/no-unprefixed-query-params': 'error', // objectui#3090 tripwire — the spec's FormField/FormFieldSchema are the // form-VIEW vocabulary (`field` = object-field reference), a DIFFERENT // layer from objectui's runtime form-field contract (`name` = data path); diff --git a/packages/app-shell/src/views/ObjectView.tsx b/packages/app-shell/src/views/ObjectView.tsx index 262521054f..a381b0e595 100644 --- a/packages/app-shell/src/views/ObjectView.tsx +++ b/packages/app-shell/src/views/ObjectView.tsx @@ -1681,16 +1681,31 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an } }, [realtimeMessage, hasConflicts, resolveAllConflicts]); - // Fetch record count for footer display + // Fetch record count for footer display. + // + // `$top: 0` IS the request: give me the total, send no rows. It was written + // `limit: 0`, which is not a `QueryParams` key — `convertQueryParams` copies + // exactly the `$`-prefixed keys the type declares — so the cap reached no + // branch and was dropped, and the platform's GET list route has no default + // page size. The effect therefore did the opposite of what it says: it + // downloaded EVERY row of the object, on every mount and every refresh of + // every list view, to read one integer off the envelope. The dead spelling + // type-checked because `QueryParams` carries `[key: string]: any` for + // adapter-specific params; `object-ui/no-unprefixed-query-params` rejects it + // at write time now (objectui#5458). + // + // `total` is the only field that can still answer the question, so the + // row-counting fallbacks are gone rather than repointed. Once we ask for + // zero rows an empty `data` means "you asked for none", not "the object is + // empty" — counting the response would report a confident `0` for any + // adapter that does not send a total. Leaving `recordCount` `undefined` + // omits the footer line instead of asserting a wrong number; the render is + // already guarded by `typeof recordCount === 'number'`. useEffect(() => { if (dataSource?.find && objectDef.name) { - dataSource.find(objectDef.name, { limit: 0 }).then((result: any) => { + dataSource.find(objectDef.name, { $top: 0 }).then((result: any) => { if (typeof result?.total === 'number') { setRecordCount(result.total); - } else if (Array.isArray(result?.data)) { - setRecordCount(result.data.length); - } else if (Array.isArray(result)) { - setRecordCount(result.length); } }).catch(() => { // Silently ignore — record count is non-critical diff --git a/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx b/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx index 4650b714e5..59aea0512d 100644 --- a/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx +++ b/packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx @@ -106,7 +106,7 @@ export function AssignedUsersSection({ permissionSetName }: AssignedUsersSection setLoading(true); try { const sets = asArray( - await adapter.find('sys_permission_set', { $filter: { name: permissionSetName }, limit: 1 }), + await adapter.find('sys_permission_set', { $filter: { name: permissionSetName }, $top: 1 }), ); const id = sets[0]?.id ? String(sets[0].id) : null; setSetId(id); diff --git a/packages/plugin-dashboard/src/DashboardFilterBar.tsx b/packages/plugin-dashboard/src/DashboardFilterBar.tsx index 8d1998b440..428e9e725e 100644 --- a/packages/plugin-dashboard/src/DashboardFilterBar.tsx +++ b/packages/plugin-dashboard/src/DashboardFilterBar.tsx @@ -259,13 +259,27 @@ function SelectFilter({ def, value, onChange, dataSource }: { def: DashboardFilt } dataSource .find(from.object, { - fields: [from.valueField, ...(from.labelField ? [from.labelField] : [])], + // `$select` / `$top`, not `fields` / `top` (objectui#5458). + // `convertQueryParams` copies exactly the `$`-prefixed keys + // `QueryParams` declares, so BOTH unprefixed spellings here reached no + // branch and were dropped: this "best-effort client-side dedupe (top + // 200 records)" was in fact fetching every row AND every column of the + // source object. Nothing rejected it — the index signature exists for + // adapter-specific params, so both keys type-checked. + // Deduped: `valueField === labelField` is the common case (both + // default to the same column), and this projection only started + // reaching the wire when the key was corrected above — so a repeated + // entry would be a NEW thing to send, not a pre-existing one. + $select: [...new Set([from.valueField, ...(from.labelField ? [from.labelField] : [])])], ...(from.filter ? { $filter: from.filter } : {}), - top: 200, + $top: 200, }) .then((records: any) => { if (cancelled) return; - const rows: any[] = Array.isArray(records) ? records : records?.items ?? []; + // `QueryResult` carries `data` — never `items`, which is not a member + // of the contract. With a real adapter the old read resolved to `[]`, + // so this fallback path produced NO options at all (objectui#5458). + const rows: any[] = Array.isArray(records) ? records : records?.data ?? []; // Real records, not an aggregate answer: `r[valueField]` IS the // stored value here, so there is no raw sidecar to pair with. setDynamicOptions(pairOptionRows(rows, undefined, from)); diff --git a/packages/plugin-dashboard/src/__tests__/DashboardFilterBar.options.test.tsx b/packages/plugin-dashboard/src/__tests__/DashboardFilterBar.options.test.tsx index d4f76b53df..614ef84e12 100644 --- a/packages/plugin-dashboard/src/__tests__/DashboardFilterBar.options.test.tsx +++ b/packages/plugin-dashboard/src/__tests__/DashboardFilterBar.options.test.tsx @@ -14,11 +14,33 @@ */ import * as React from 'react'; -import { describe, it, expect, vi, afterEach } from 'vitest'; -import { render, screen, cleanup, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi, beforeAll, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react'; import { DashboardFilterBar } from '../DashboardFilterBar'; import type { DashboardFilterDef } from '@object-ui/core'; +// Radix Select opens on pointer events jsdom does not implement — the same shim +// `DashboardFilterBar.optionsFromRawValue.test.tsx` uses. Needed because the +// option list only exists in the DOM once the popup is open, so any assertion +// about WHICH options were derived has to open it. +beforeAll(() => { + class MockPointerEvent extends Event { + button: number; + ctrlKey: boolean; + pointerType: string; + constructor(type: string, props: any = {}) { + super(type, props); + this.button = props.button ?? 0; + this.ctrlKey = props.ctrlKey ?? false; + this.pointerType = props.pointerType ?? 'mouse'; + } + } + (window as any).PointerEvent = MockPointerEvent; + (HTMLElement.prototype as any).hasPointerCapture = vi.fn(); + (HTMLElement.prototype as any).releasePointerCapture = vi.fn(); + (HTMLElement.prototype as any).scrollIntoView = vi.fn(); +}); + afterEach(cleanup); const defs: DashboardFilterDef[] = [ @@ -74,7 +96,39 @@ describe('DashboardFilterBar — optionsFrom fetching', () => { renderBar({ queryDataset, find }); await waitFor(() => expect(find).toHaveBeenCalledTimes(1)); - expect(find).toHaveBeenCalledWith('accounts', expect.objectContaining({ top: 200 })); + // The EXACT params, not `objectContaining` (objectui#5458). This assertion + // read `{ top: 200 }` until that card: `top` and `fields` are not + // `QueryParams` keys, `convertQueryParams` copies only the `$`-prefixed + // ones, so BOTH were dropped and this "top-200 dedupe" in fact scanned + // every row and every column of `accounts`. A partial matcher is what let + // the dead spelling sit here looking asserted, so the whole object is + // pinned now and a re-introduced bare key fails. + expect(find).toHaveBeenCalledWith('accounts', { $select: ['industry'], $top: 200 }); + }); + + it('reads options off a QueryResult envelope, not a bare array', async () => { + // The shape a REAL adapter returns: `find()` resolves to `QueryResult`, + // whose records live under `data`. This path used to read `records.items` + // — not a member of the contract — so with any real data source the + // fallback resolved to `[]` and the filter offered NO options at all + // (objectui#5458). Every other case in this file mocks a bare array, which + // took the `Array.isArray` arm and never exercised this one. + const queryDataset = vi.fn().mockRejectedValue(new Error('datasets unsupported')); + const find = vi.fn().mockResolvedValue({ + data: [{ industry: 'finance' }, { industry: 'retail' }], + total: 2, + }); + renderBar({ queryDataset, find }); + + await waitFor(() => expect(find).toHaveBeenCalledTimes(1)); + + fireEvent.pointerDown(screen.getByTestId('dashboard-filter-industry'), { button: 0 }); + // PRE-FIX these two `findByRole` calls both time out: `records.items` was + // `undefined` on a `QueryResult`, so `rows` fell to `[]` and the dropdown + // opened empty. Reading the option list is what discriminates — asserting + // that the control renders passes either way. + expect(await screen.findByRole('option', { name: 'finance' })).toBeInTheDocument(); + expect(await screen.findByRole('option', { name: 'retail' })).toBeInTheDocument(); }); it('uses the client-side path directly when the data source has no queryDataset', async () => {