diff --git a/.changeset/6286-explain-batch-cap-from-spec.md b/.changeset/6286-explain-batch-cap-from-spec.md new file mode 100644 index 0000000000..67e18ef880 --- /dev/null +++ b/.changeset/6286-explain-batch-cap-from-spec.md @@ -0,0 +1,40 @@ +--- +'@object-ui/plugin-grid': patch +--- + +The batch-explain cap the row-verdict hook paginates under is now imported from +`@objectstack/spec/security` instead of re-declared locally (objectui#6286). +`useRecordCrudVerdicts` carried `const EXPLAIN_BATCH_MAX_RECORD_IDS = 200`, a hand copy of +a SERVER contract constant, under a doc comment that named its own expiry condition: the +pinned `@objectstack/spec@17.0.0-rc.6` predated the batch form, and the pin bump would +supersede the declaration. It has. + +**No value changes and no behaviour changes.** The spec exports `200`, which is what the +local copy said, verified by resolving the installed package and reading the export — both +statically (`dist/security/index.d.mts`) and at runtime through the same specifier the +source now uses. What changes is reference identity: if the server relaxes or tightens the +cap and the spec follows, the client follows too, instead of paginating at the old boundary +with no signal anywhere. The cap's whole point is that an over-cap request is refused with +`400 VALIDATION_FAILED` rather than truncated, so a client that silently disagrees with it +is exactly the drift `scripts/check-spec-symbol-derivation.mjs` argues about — and could +not catch here, because both of its scanners skip non-exported declarations and this const +was module-local (objectui#5899). + +The declared floor already carries the symbol, so no range moves: `@objectstack/spec@17.0.0` +— the minimum `^17.0.0` admits — exports `EXPLAIN_BATCH_MAX_RECORD_IDS = 200` from +`./security`. Measured against the published tarballs of `17.0.0-rc.6`, `17.0.0`, `17.1.0` +and `17.2.0`: only the rc lacks it. The declaration was therefore expired one release +earlier than the card that found it assumed. + +The half of the comment that explains *why* the cap exists and what the server does with an +over-cap request is kept and now sits on the import; only the half explaining why it was +declared LOCALLY is gone, since that is the part that stopped being true. + +Covered by a new reference-identity test rather than a value assertion. Every assertion on +`200` passes on both sides of this change — a ghost — so +`useRecordCrudVerdicts.batchCap.test.tsx` stands the spec module in at a cap no hand copy +could produce and asserts the request chunking follows it, with a control case proving the +stand-in installed and differs from the shipped value. The pre-existing cap assertion in +`rowRecordCrudVerdict.test.tsx` now derives its fixture and its bound from the same export +instead of re-typing `200`, which removes the last hand copy on this surface without +pretending to be a two-world test. diff --git a/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx b/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx index 67c4b36302..69a492a3b3 100644 --- a/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx +++ b/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx @@ -37,6 +37,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EXPLAIN_BATCH_MAX_RECORD_IDS } from '@objectstack/spec/security'; import { render, screen, waitFor, cleanup, act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import '@testing-library/jest-dom'; @@ -444,20 +445,28 @@ describe('[#4296] one batched call per (object, operation) per page', () => { } }); - it('paginates under the server\'s 200-id cap instead of sending a request it would refuse', async () => { - // The cap is the server's (`EXPLAIN_BATCH_MAX_RECORD_IDS`): over-cap + it('paginates under the server\'s cap instead of sending a request it would refuse', async () => { + // The cap is the server's (`EXPLAIN_BATCH_MAX_RECORD_IDS`, imported from + // the package that owns it rather than re-typed — objectui#6286): over-cap // requests are refused with 400 VALIDATION_FAILED, never truncated, and the // spec directs consumers to paginate under it. Still amortized — 2 calls - // per operation for 250 rows, not 250. - const rows = bigPage(250); + // per operation, never one per row. + // + // The fixture is sized FROM the cap: `cap + m` rows with `0 < m <= cap` is + // exactly two chunks per operation for ANY cap, so this stays a cap test if + // the contract moves. That does NOT make it a two-world test for the + // import itself — the value is the same on both sides of that change, which + // is what `../hooks/useRecordCrudVerdicts.batchCap.test.tsx` exists to pin. + const cap = EXPLAIN_BATCH_MAX_RECORD_IDS; + const rows = bigPage(cap + Math.min(50, cap)); for (const r of rows) server.verdicts.set(r.id, { update: true, delete: true }); renderGrid({ rows }); - await waitFor(() => expect(screen.getByText('Row 249')).toBeInTheDocument()); + await waitFor(() => expect(screen.getByText(`Row ${rows.length - 1}`)).toBeInTheDocument()); await settle(4); expect(server.calls.length).toBe(4); for (const call of server.calls) { - expect(call.recordIds!.length).toBeLessThanOrEqual(200); + expect(call.recordIds!.length).toBeLessThanOrEqual(cap); expect(call.recordIds!.length).toBeGreaterThan(0); } for (const operation of ['update', 'delete'] as const) { diff --git a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx new file mode 100644 index 0000000000..c71a41c09e --- /dev/null +++ b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx @@ -0,0 +1,166 @@ +/** + * 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#6286] The cap this hook paginates under IS + * `@objectstack/spec/security`'s `EXPLAIN_BATCH_MAX_RECORD_IDS` — not a local + * number that happens to equal it. + * + * ## Why this file exists, and why asserting the VALUE would not do + * + * Until this card the hook declared `const EXPLAIN_BATCH_MAX_RECORD_IDS = 200` + * locally, and the spec exports `200`. So every assertion on the value passes + * in BOTH worlds — with the hand copy and with the import. That includes the + * cap assertion in `../__tests__/rowRecordCrudVerdict.test.tsx`, which reads + * like a cap test and cannot fail for a drifted cap. A ghost assertion. + * + * What the fix actually changes is REFERENCE IDENTITY: the client can no + * longer drift from the server contract. The only honest way to pin that is to + * MOVE the spec's export and watch the hook follow — so this file stands the + * spec module in at a cap no hand copy could produce and asserts the request + * chunking tracks it. Against the pre-fix hook the stand-in is inert by + * construction (that module is not in its import graph at all) and every case + * below fails: it would send one request of seven ids where three are due. + * + * The stand-in is a bare package specifier, which + * `scripts/check-vi-mock-specifiers.mjs` does not judge (it resolves RELATIVE + * specifiers only), so "the mock silently did not install" is guarded here + * instead — see the control case at the bottom. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderHook, waitFor } from '@testing-library/react'; + +const { STUB_CAP, probe } = vi.hoisted(() => ({ + /** + * Deliberately nothing like the real cap, and small enough that the fixture + * stays readable. A hook reading its own copy of the contract cannot produce + * this chunking under any value the spec has ever shipped. + */ + STUB_CAP: 3, + /** + * What the stand-in observed on its way past: whether the factory ran at all + * (i.e. something in the hook's graph really imports this module), and what + * the REAL contract says (so the stub cannot be accidentally equal to it). + */ + probe: { factoryRan: false, realCap: undefined as unknown }, +})); + +vi.mock('@objectstack/spec/security', async (importOriginal) => { + const actual = await importOriginal(); + probe.factoryRan = true; + probe.realCap = actual.EXPLAIN_BATCH_MAX_RECORD_IDS; + return { ...actual, EXPLAIN_BATCH_MAX_RECORD_IDS: STUB_CAP }; +}); + +import { useRecordCrudVerdicts, __clearRecordCrudVerdictCache } from './useRecordCrudVerdicts'; + +const OBJECT = 'showcase_project'; + +interface ExplainCall { + object?: string; + operation?: string; + recordIds?: string[]; +} + +let calls: ExplainCall[] = []; + +/** Records every explain request and answers it `visible: true`. */ +function stubExplain() { + vi.stubGlobal( + 'fetch', + vi.fn(async (_input: unknown, init?: { body?: unknown }) => { + const body = JSON.parse(String(init?.body ?? '{}')) as ExplainCall; + calls.push(body); + return { + ok: true, + status: 200, + json: async () => ({ + allowed: true, + object: body.object, + operation: body.operation, + records: (body.recordIds ?? []).map((recordId) => ({ recordId, visible: true })), + }), + }; + }), + ); +} + +const idsFor = (n: number) => Array.from({ length: n }, (_, i) => `r_${i}`); + +describe('[#6286] the batch cap is the spec\'s export, not a local copy', () => { + beforeEach(() => { + // Module-level memo — without this, ids answered by an earlier case are + // "not missing" in the next one and the chunk arithmetic silently changes. + __clearRecordCrudVerdictCache(); + calls = []; + stubExplain(); + }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('splits a page at the SPEC\'s cap — chunk sizes follow the export when it moves', async () => { + const ids = idsFor(STUB_CAP * 2 + 1); + renderHook(() => useRecordCrudVerdicts({ objectName: OBJECT, recordIds: ids, update: true })); + + await waitFor(() => expect(calls.length).toBe(3)); + // The pre-fix hook produces exactly one call of seven ids here. + expect(calls.map((c) => c.recordIds?.length)).toEqual([STUB_CAP, STUB_CAP, 1]); + // Splitting must not drop or reorder a row: every id asked exactly once. + expect(calls.flatMap((c) => c.recordIds ?? [])).toEqual(ids); + expect(calls.every((c) => c.object === OBJECT && c.operation === 'update')).toBe(true); + }); + + it('splits only ABOVE the cap — exactly cap ids is one request, cap + 1 is two', async () => { + // BOTH sides are asserted deliberately. "Exactly cap ids is one request" is + // true of the pre-fix hook too (3 ids fit under a cap of 200), so on its own + // it is another ghost — it is the `cap + 1` half that can only pass when the + // boundary being applied is the spec's. The ablation leg for this file found + // that out: written with the first half alone, this case survived the revert. + const atCap = idsFor(STUB_CAP); + const { unmount } = renderHook(() => + useRecordCrudVerdicts({ objectName: OBJECT, recordIds: atCap, update: true }), + ); + await waitFor(() => expect(calls.length).toBe(1)); + expect(calls[0].recordIds).toEqual(atCap); + + unmount(); + __clearRecordCrudVerdictCache(); + calls = []; + + const overCap = idsFor(STUB_CAP + 1); + renderHook(() => useRecordCrudVerdicts({ objectName: OBJECT, recordIds: overCap, update: true })); + await waitFor(() => expect(calls.length).toBe(2)); + expect(calls.map((c) => c.recordIds?.length)).toEqual([STUB_CAP, 1]); + }); + + it('splits per operation, so two verbs over cap+1 ids cost four requests', async () => { + const ids = idsFor(STUB_CAP + 1); + renderHook(() => + useRecordCrudVerdicts({ objectName: OBJECT, recordIds: ids, update: true, delete: true }), + ); + + await waitFor(() => expect(calls.length).toBe(4)); + for (const operation of ['update', 'delete'] as const) { + const forOp = calls.filter((c) => c.operation === operation); + expect(forOp.map((c) => c.recordIds?.length)).toEqual([STUB_CAP, 1]); + } + }); + + it('control: the stand-in really installed, and it differs from the real contract', () => { + // If this is false the three cases above proved nothing about provenance — + // they would be measuring the real cap under a different name. It is true + // only because the hook's own import graph pulled this module in. + expect(probe.factoryRan).toBe(true); + // A stub that happened to equal the shipped cap would make every assertion + // above pass for the pre-fix hook too — the ghost this file exists to avoid. + expect(typeof probe.realCap).toBe('number'); + expect(probe.realCap).not.toBe(STUB_CAP); + }); +}); diff --git a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts index b8af8141c3..5065876bd5 100644 --- a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts +++ b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts @@ -54,24 +54,23 @@ * cookie-session hosts. */ import * as React from 'react'; -import { SchemaRendererContext } from '@object-ui/react'; - -/** The two write verbs a list row's kebab can offer. */ -export type RecordCrudOperation = 'update' | 'delete'; - /** * Hard cap on `recordIds` per batch explain request — the SERVER's contract * (`EXPLAIN_BATCH_MAX_RECORD_IDS`, objectstack#8326): over-cap requests are * refused with `400 VALIDATION_FAILED`, never truncated, and the spec's own * TSDoc directs a consumer with more records to paginate under it. A page - * larger than the cap is therefore split into ceil(N / 200) requests per + * larger than the cap is therefore split into ceil(N / cap) requests per * operation — still amortized, never one per row. * - * Declared locally because the pinned `@objectstack/spec@17.0.0-rc.6` predates - * the batch form and exports neither the constant nor the request/response - * types; the pin bump (objectui#4636) supersedes this declaration. + * Imported from the package that OWNS the contract, never re-declared: a hand + * copy passes every value comparison on the day it is written and drifts + * silently on the day the server moves the cap. */ -const EXPLAIN_BATCH_MAX_RECORD_IDS = 200; +import { EXPLAIN_BATCH_MAX_RECORD_IDS } from '@objectstack/spec/security'; +import { SchemaRendererContext } from '@object-ui/react'; + +/** The two write verbs a list row's kebab can offer. */ +export type RecordCrudOperation = 'update' | 'delete'; /** * One entry of the batch response's `records` array, narrowed to what this hook