diff --git a/.changeset/9928-saved-view-limit-non-positive.md b/.changeset/9928-saved-view-limit-non-positive.md new file mode 100644 index 0000000000..813ccec30a --- /dev/null +++ b/.changeset/9928-saved-view-limit-non-positive.md @@ -0,0 +1,44 @@ +--- +'@object-ui/core': minor +--- + +Drop a saved view's row cap that the contract refuses, at the LOWERING layer every +repaired read point sits under — and say so on the one path that has no renderer to +say it (objectui#9928). + +`savedViewLimit` admitted its carrier on `typeof … === 'number'` alone, so a saved +view's `pagination.pageSize` (or its legacy flat `limit`) of `0`, `-10` or `25.5` +lowered unchecked into the composed `limit`. Both ends of that journey are declared +positive — the spec's element data source declares `limit` a positive integer, and a +view's pagination declares `pageSize` a positive integer with a default — so the +lowering layer in between was the one place that asked nothing. + +The two consumers of the composed key behave differently, and both were measured +rather than inferred: + +- through a RENDERER, the refused value reached a block that has its own guard, so + the block dropped it and drew its own default — the named view's cap went missing + and the read went **wider** than the view asked for, in the one direction a named + view exists to prevent; +- through `ViewDataProvider.resolveElementDataSource`, which forwards this key + straight to `DataFetcher.fetchRecords` with **no guard of its own**, `0`, `-10` and + `25.5` reached the fetcher verbatim and nothing anywhere said so. + +The cap is now **dropped**, not clamped and not thrown. Clamping was refused because +this layer has no default to clamp to — every consuming block owns its own default +and `ViewDataProvider` owns none, so a number invented here would override a default +the author never asked it to. Throwing was refused because the composer is pure and +sits under every block that can be bound to a view. + +Dropping alone would have been silent, and worse than silent: the renderer that used +to report the refused value now receives nothing and correctly says nothing, so the +repair would have removed the only place the author was being told. New export +`elementDataSourceRefusedLimitMessage` is the loud half — a pure builder, following +the shape `elementDataSourceViewNotFoundMessage` already established in this module so +that every caller reports the same defect the same way. `ViewDataProvider` reports it +on the `console.warn` channel the repaired read points use; the value is fail-soft, so +records still load. + +Unchanged: carrier precedence (a non-numeric `pagination.pageSize` still falls through +to the flat `limit`), every other composed key, and the binding's own `limit`, which +overrides a view cap exactly as before. diff --git a/packages/core/src/data-scope/ViewDataProvider.ts b/packages/core/src/data-scope/ViewDataProvider.ts index ff5fb85c9d..e623916d07 100644 --- a/packages/core/src/data-scope/ViewDataProvider.ts +++ b/packages/core/src/data-scope/ViewDataProvider.ts @@ -23,6 +23,7 @@ import { collectSavedViews, composeElementDataSource, + elementDataSourceRefusedLimitMessage, elementDataSourceViewNotFoundMessage, resolveSavedView, type ElementDataSourceConfig, @@ -351,6 +352,19 @@ export class ViewDataProvider { } const composed = composeElementDataSource(config, view); + + // The loud half of the row-cap refusal. This method is the consumer that + // has NO guard of its own — it forwards `limit` straight to + // `DataFetcher.fetchRecords` — so before the guard a view's `0` / `-10` / + // `25.5` reached the fetcher verbatim, and nothing anywhere said so. A + // renderer that receives the same key reports it through its own channel; + // this path has no renderer, so it reports here, on the same `console.warn` + // channel those sites use. ⛔ Not an `error`: the refusal is FAIL-SOFT and + // the records still load, so blanking the result would be a worse outcome + // than the defect. + const refusedLimit = elementDataSourceRefusedLimitMessage(view, config.view, config.object); + if (refusedLimit) console.warn(refusedLimit); + const fields = Array.isArray(composed.columns) ? composed.columns.filter((c): c is string => typeof c === 'string' && !!c) : undefined; diff --git a/packages/core/src/data-scope/__tests__/element-data-source.savedViewLimitNonPositive-9928.test.ts b/packages/core/src/data-scope/__tests__/element-data-source.savedViewLimitNonPositive-9928.test.ts new file mode 100644 index 0000000000..124a40826e --- /dev/null +++ b/packages/core/src/data-scope/__tests__/element-data-source.savedViewLimitNonPositive-9928.test.ts @@ -0,0 +1,221 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * objectui#9928 — a saved view's row cap that the contract refuses must not be + * lowered into the composed `limit`. + * + * `savedViewLimit` admitted its carrier on `typeof … === 'number'` alone, so + * `0`, negatives and fractions lowered unchecked. This is the LOWERING layer + * that sits under the repaired read points, and the value it produces has two + * consumers that behave differently — which is why both are pinned here: + * + * - a RENDERER, which has a guard of its own, so the refused value was dropped + * there and the block drew its own default: the named view's cap went + * missing and the read went WIDER than the view asked for; + * - `ViewDataProvider.resolveElementDataSource`, which forwards this key + * straight to `DataFetcher.fetchRecords` with NO guard of its own, so the + * refused value reached the fetcher verbatim and nothing said so. + * + * ⚠️ Every CONTROL in this file passes both BEFORE and AFTER the repair. That + * is deliberate: a change that simply stopped lowering any view cap at all + * would satisfy the refusals and fail the controls. + * + * ⚠️ KNOWN GAP, deliberately not pinned here: the BINDING's own `limit` — the + * other operand of `config.limit ?? savedViewLimit(view)` — is still admitted + * unchecked, so `dataSource: { object, limit: 0 }` still reaches the fetcher as + * `0`. That carrier raises a PRECEDENCE question this card does not own (does a + * refused binding cap suppress the view's legitimate one?), and it is reported + * rather than answered here. Nothing in this file asserts the current answer, + * so the card that settles it will not have to edit a pin that endorsed it. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { + composeElementDataSource, + elementDataSourceRefusedLimitMessage, +} from '../element-data-source'; +import { ViewDataProvider, type DataFetcher } from '../ViewDataProvider'; + +/** The values the contract refuses, in both carriers a saved view may use. */ +const REFUSED = [0, -10, 25.5, -0.5] as const; + +const bind = { object: 'account', view: 'hot' } as const; + +describe('objectui#9928 — savedViewLimit drops a cap the contract refuses', () => { + describe('the composed `limit`', () => { + it.each(REFUSED)('`pagination.pageSize: %s` is not lowered', (bad) => { + const composed = composeElementDataSource(bind, { pagination: { pageSize: bad } }); + expect(composed.limit).toBeUndefined(); + expect('limit' in composed).toBe(false); + }); + + it.each(REFUSED)('a flat `limit: %s` is not lowered either', (bad) => { + // The same function's second carrier. The card's excerpt stopped above + // this branch, but it admits on `typeof … === 'number'` in exactly the + // same way, so leaving it would have left the defect reachable by the + // legacy spelling. + const composed = composeElementDataSource(bind, { limit: bad }); + expect(composed.limit).toBeUndefined(); + expect('limit' in composed).toBe(false); + }); + + // ---------------------------------------------------------------- CONTROLS + it('CONTROL — a legitimate `pagination.pageSize` still lowers', () => { + expect(composeElementDataSource(bind, { pagination: { pageSize: 7 } }).limit).toBe(7); + expect(composeElementDataSource(bind, { pagination: { pageSize: 25 } }).limit).toBe(25); + }); + + it('CONTROL — a legitimate flat `limit` still lowers', () => { + expect(composeElementDataSource(bind, { limit: 7 }).limit).toBe(7); + }); + + it('CONTROL — the binding still overrides a legitimate view cap', () => { + const composed = composeElementDataSource( + { object: 'account', view: 'hot', limit: 5 }, + { pagination: { pageSize: 7 } }, + ); + expect(composed.limit).toBe(5); + }); + + it('CONTROL — a view with no cap at all still composes without one', () => { + expect(composeElementDataSource(bind, { label: 'Hot' }).limit).toBeUndefined(); + expect(composeElementDataSource({ object: 'account' }).limit).toBeUndefined(); + }); + + it('CONTROL — carrier PRECEDENCE is unchanged: a non-numeric `pagination.pageSize` still falls through to the flat `limit`', () => { + // Only the positivity question is new. The `typeof === 'number'` + // selection that picks BETWEEN the two carriers is untouched, so a view + // storing a string page size keeps resolving to its flat `limit`. + const composed = composeElementDataSource(bind, { + pagination: { pageSize: 'twenty' }, + limit: 20, + }); + expect(composed.limit).toBe(20); + }); + + it('CONTROL — the other composed keys compose as before when the cap is usable', () => { + const composed = composeElementDataSource(bind, { + pagination: { pageSize: 7 }, + columns: ['name'], + type: 'kanban', + }); + expect(composed.columns).toEqual(['name']); + expect(composed.viewType).toBe('kanban'); + expect(composed.limit).toBe(7); + }); + + // ⚠️ Deliberately NOT labelled a control: it asserts the drop, so it is one + // of the tests that must go red when the guard is ablated. The reverse + // verification caught an earlier version of this file calling it a control + // — every name carrying CONTROL or SILENCE below passes on both sides of + // the guard, and this one does not. + it('leaves the other composed keys alone while dropping the cap', () => { + const composed = composeElementDataSource(bind, { + pagination: { pageSize: 0 }, + columns: ['name'], + type: 'kanban', + }); + expect(composed.columns).toEqual(['name']); + expect(composed.viewType).toBe('kanban'); + expect(composed.limit).toBeUndefined(); + }); + }); + + describe('the message — the half that tells the author', () => { + it.each(REFUSED)('names the refused value %s, the view and the object', (bad) => { + const msg = elementDataSourceRefusedLimitMessage( + { pagination: { pageSize: bad } }, + 'hot', + 'account', + ); + expect(msg).toContain(String(bad)); + expect(msg).toContain('hot'); + expect(msg).toContain('account'); + expect(msg).toContain('positive integer'); + }); + + it('reports the flat carrier too', () => { + expect(elementDataSourceRefusedLimitMessage({ limit: 0 }, 'hot', 'account')) + .toContain('row cap of 0'); + }); + + // -------------------------------------------------------- SILENCE CONTROLS + // Without these the message is an always-on marker that states nothing. + it('SILENCE — says nothing when the view carries a usable cap', () => { + expect(elementDataSourceRefusedLimitMessage({ pagination: { pageSize: 7 } }, 'hot', 'account')) + .toBeNull(); + expect(elementDataSourceRefusedLimitMessage({ limit: 7 }, 'hot', 'account')).toBeNull(); + }); + + it('SILENCE — says nothing when the view carries no cap at all', () => { + expect(elementDataSourceRefusedLimitMessage({ label: 'Hot' }, 'hot', 'account')).toBeNull(); + expect(elementDataSourceRefusedLimitMessage({ pagination: {} }, 'hot', 'account')).toBeNull(); + }); + + it('SILENCE — says nothing when there is no view', () => { + expect(elementDataSourceRefusedLimitMessage(null, 'hot', 'account')).toBeNull(); + expect(elementDataSourceRefusedLimitMessage(undefined, undefined, 'account')).toBeNull(); + }); + + it('SILENCE — says nothing about a carrier this layer never lowered', () => { + // A non-numeric page size did not become a limit before this repair and + // does not now, so this layer has nothing to report about it. + expect(elementDataSourceRefusedLimitMessage( + { pagination: { pageSize: 'twenty' } }, + 'hot', + 'account', + )).toBeNull(); + }); + + it('still names the object when the view name is absent', () => { + const msg = elementDataSourceRefusedLimitMessage({ limit: -1 }, undefined, 'account'); + expect(msg).toContain('account'); + expect(msg).toContain('-1'); + }); + }); + + describe('ViewDataProvider — the consumer with no guard of its own', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + const resolveWith = async (viewConfig: Record) => { + const fetchRecords = vi.fn( + async () => ({ records: [], total: 0 }), + ); + const provider = new ViewDataProvider(); + provider.setFetcher({ + fetchRecords, + fetchViews: async () => ({ hot: viewConfig }), + }); + await provider.resolveElementDataSource({ object: 'account', view: 'hot' }); + return fetchRecords.mock.calls[0]?.[1]; + }; + + it.each(REFUSED)('does not forward %s to the fetcher', async (bad) => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const options = await resolveWith({ pagination: { pageSize: bad } }); + expect(options?.limit).toBeUndefined(); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0])).toContain(String(bad)); + }); + + it('CONTROL — forwards a legitimate cap, and says nothing', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const options = await resolveWith({ pagination: { pageSize: 7 } }); + expect(options?.limit).toBe(7); + expect(warn).not.toHaveBeenCalled(); + }); + + it('CONTROL — says nothing for a view that declares no cap', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const options = await resolveWith({ label: 'Hot' }); + expect(options?.limit).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/core/src/data-scope/element-data-source.ts b/packages/core/src/data-scope/element-data-source.ts index df19d5f246..8104e38b5d 100644 --- a/packages/core/src/data-scope/element-data-source.ts +++ b/packages/core/src/data-scope/element-data-source.ts @@ -43,6 +43,12 @@ * | `filter` | view + binding | AND-combined ("additional") | * | `sort` | view or binding | binding overrides view | * | `limit` | view (`pagination.pageSize`) or binding | binding overrides view | + * + * The view's half of `limit` carries one extra condition the other keys do not: + * the destination is declared a POSITIVE INTEGER, so a view's cap the contract + * refuses is dropped rather than lowered. See `savedViewLimit` for why dropping + * beats clamping or throwing here, and `elementDataSourceRefusedLimitMessage` + * for the half that tells the author. * | `viewType` | view only | view | * * A lone `filter` — only the view has one, or only the binding does — is passed @@ -99,7 +105,11 @@ export interface ComposedElementDataSource { filter?: unknown; /** Binding sort if given, else the view's. */ sort?: unknown; - /** Binding limit if given, else the view's page size. */ + /** + * Binding limit if given, else the view's page size — and, from the view, + * only a cap the contract admits (`savedViewLimit` drops the rest, and + * `elementDataSourceRefusedLimitMessage` is what says so). + */ limit?: number; /** The view's render kind (grid / kanban / …), when the view declares one. */ viewType?: string; @@ -186,8 +196,43 @@ export function resolveSavedView( return id === undefined ? undefined : views[id]; } -/** Read a saved view's row cap — `pagination.pageSize`, or a flat `limit`. */ -function savedViewLimit(view: ElementSavedView | null | undefined): number | undefined { +/** + * What the contract admits as a row cap. + * + * A deliberate LOCAL RESTATEMENT of the predicate the repaired read points + * spell as `isUsableRowLimit` / `isUsablePageSize`. It is ⛔ not imported and + * ⛔ not extracted to a shared helper: this family now spells the same rule at + * each site that enforces it, and `isUsableRowLimit` / `isUsablePageSize` is + * what enumerates them. Hoisting it into one module would be a cross-package + * move nobody has chartered, and it would take the rule out of the file whose + * reader needs to see it. The cost — one more copy that could drift — is + * written here rather than hidden. + * + * The destination is declared positive at both ends: the spec's element data + * source declares `limit` a positive integer, and a saved view's pagination + * declares `pageSize` a positive integer with a default. So `0` is not a + * spelling whose meaning this layer may choose — it is a value the contract + * already refuses. + */ +function isUsableRowLimit(value: unknown): value is number { + return typeof value === 'number' && Number.isInteger(value) && value > 0; +} + +/** + * The row cap a saved view CARRIES, before the contract is applied to it — + * `pagination.pageSize`, or a flat `limit`. + * + * Split out from {@link savedViewLimit} so the resolver and the diagnostic read + * the same carrier by construction. Two readers would be free to drift, and the + * drift would be invisible: a value one of them reports and the other keeps, or + * the reverse. + * + * ⚠️ Selection is still by `typeof === 'number'`, unchanged: a non-numeric + * `pagination.pageSize` falls through to the flat `limit` exactly as it did + * before. Only the POSITIVITY question is new, and it is asked once, by + * {@link isUsableRowLimit}, on whichever carrier won. + */ +function savedViewRawLimit(view: ElementSavedView | null | undefined): number | undefined { if (!view) return undefined; const pagination = view.pagination; if (isPlainObject(pagination) && typeof pagination.pageSize === 'number') { @@ -196,6 +241,93 @@ function savedViewLimit(view: ElementSavedView | null | undefined): number | und return typeof view.limit === 'number' ? view.limit : undefined; } +/** + * Read a saved view's row cap — `pagination.pageSize`, or a flat `limit` — + * and hand back only a cap the contract admits. + * + * ## Why DROP, and not clamp or refuse + * + * Before this guard, `typeof … === 'number'` admitted `0`, negatives and + * fractions, and they lowered unchecked into the composed `limit`. Measured on + * both consumers of that key rather than inferred, and the two answers differ: + * + * - through a RENDERER, the refused value reached a block that has its own + * guard, so the block dropped it and drew its own default — the named view's + * cap went missing and the read went WIDER than the view asked for; + * - through `ViewDataProvider.resolveElementDataSource`, which forwards this + * key to `DataFetcher.fetchRecords` with NO guard of its own, `0`, `-10` and + * `25.5` reached the fetcher verbatim. + * + * ⛔ CLAMP is refused: this layer has no default to clamp to. Every consuming + * block owns its own default and `ViewDataProvider` owns none, so a number + * invented here would override a default the author never asked it to, and + * quietly substituting a number the author never wrote is the quieter half of + * this same defect. + * + * ⛔ THROWING is refused: this function is pure and sits under every block that + * can be bound to a view, so a throw would take out the page over one + * declaration — worse than the defect. + * + * ⇒ DROP. An absent `limit` is the honest statement that the view supplied no + * usable cap, and it is the one thing every consumer already handles. + * + * The loud half is {@link elementDataSourceRefusedLimitMessage}; see its doc for + * why the message is built here and reported by the caller. + */ +function savedViewLimit(view: ElementSavedView | null | undefined): number | undefined { + const raw = savedViewRawLimit(view); + return isUsableRowLimit(raw) ? raw : undefined; +} + +/** + * The diagnostic half of {@link savedViewLimit}. `null` means "nothing to say". + * + * ## Why a BUILDER here, and not a warning from the composer + * + * This is the shape {@link elementDataSourceViewNotFoundMessage} already + * established in this module, for the reason its doc gives: built here so every + * caller reports the same defect the same way. It is followed rather than + * re-decided because the alternative is ruled out mechanically — + * {@link composeElementDataSource} is PURE and the render path calls it from a + * `useMemo`, so a warning emitted inside it would fire during render and fire + * again on every re-render. The repaired relay one layer up emits from an + * effect for exactly that reason; a pure function has no effect to emit from, + * so it hands the caller the words instead. + * + * ## Why the message has to exist at all + * + * Dropping alone would be a SILENT change on the renderer path: today the + * refused value travels as far as a block whose own guard reports it once, and + * after this repair that block receives nothing and correctly says nothing — + * an absent key is not a mistake. So the repair would remove the only place the + * author was being told. The layer that makes the decision is the layer that + * reports it. + * + * ⛔ NOT a second guard: the predicate lives once, in {@link isUsableRowLimit}, + * and the carrier is read once, by {@link savedViewRawLimit}; this reads both. + * + * ⚠️ It speaks only about a cap THIS layer dropped. A non-numeric + * `pagination.pageSize` never became a limit here, before or after, so there is + * nothing for this layer to report about it. + */ +export function elementDataSourceRefusedLimitMessage( + view: ElementSavedView | null | undefined, + viewName: string | undefined | null, + object: string, +): string | null { + const raw = savedViewRawLimit(view); + if (raw === undefined) return null; + if (isUsableRowLimit(raw)) return null; + const where = viewName ? `saved view "${viewName}" on ${object}` : `saved view on ${object}`; + return ( + `[ObjectUI] ElementDataSource: ${where} declares a row cap of ${String(raw)}, ` + + 'which is not a positive integer. A row cap must be a positive integer ' + + '(the spec declares this binding’s `limit` positive, and refuses a zero ' + + 'or negative `pagination.pageSize`), so the view’s cap was ignored and no ' + + 'cap was lowered from it — the consuming block uses its own default.' + ); +} + /** * Read a saved view's render kind. `type` is the spec's key (and what * `ObjectView` reads); `viewType` is objectui's legacy spelling, folded by diff --git a/packages/core/src/data-scope/index.ts b/packages/core/src/data-scope/index.ts index bfa9fc70d2..e510da9597 100644 --- a/packages/core/src/data-scope/index.ts +++ b/packages/core/src/data-scope/index.ts @@ -28,6 +28,7 @@ export { ELEMENT_DATA_SOURCE_INPUT, ELEMENT_DATA_SOURCE_KEY, elementDataSourceBlock, + elementDataSourceRefusedLimitMessage, elementDataSourceViewNotFoundMessage, isElementDataSourceBlock, isElementDataSourceConfig,