diff --git a/.changeset/11402-dataset-blank-field.md b/.changeset/11402-dataset-blank-field.md new file mode 100644 index 0000000000..b386615143 --- /dev/null +++ b/.changeset/11402-dataset-blank-field.md @@ -0,0 +1,11 @@ +--- +'@object-ui/app-shell': patch +--- + +The dataset designer no longer writes `field: ''` for a row whose Field box is blank (objectui#11402). + +New dimension and measure rows used to be seeded with `field: ''`, so a Studio author who added a plain row-count measure and left the Field box blank saved `field: ''`. That value means nothing to any reader: the analytics query answered 500 on the ObjectQL strategy for a `count` measure carrying it, and the narrowed dataset schema (objectstack-ai/objectstack#21240) refuses it at save. + +- New rows are seeded without a `field` key, and a blank Field value removes the key instead of storing `''`. A `count` measure with no field therefore saves the one spelling the spec has for count(*): the key absent. +- A dimension needs a field. A dimension whose Field box is blank (including a stored `field: ''`) is reported to the editor's Save gate, the same channel that holds Save, autosave and the shortcut for a CEL expression that does not parse, until a field is picked. Its Field label carries the designer's required marker. +- Nothing is filled in on the author's behalf: no `id` or other default field is written. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.blankField-11402.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.blankField-11402.test.tsx new file mode 100644 index 0000000000..0307381afc --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.blankField-11402.test.tsx @@ -0,0 +1,213 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#11402 — a blank Field box writes no `field` key. + * + * The dataset inspector used to seed every new row with `field: ''`, so a + * Studio author who added a plain row-count measure and left the Field box + * blank saved `field: ''`. The spec has one spelling for "no field" on a + * measure — the key is ABSENT (`DatasetMeasureSchema.field` is optional, and + * only `count` may omit it) — and a dimension's `field` is required, so an + * empty string is a value no reader can use: the analytics door answered 500 + * on the ObjectQL strategy for a `count` measure with `field: ''`, and the + * narrowed spec (objectstack-ai/objectstack#21240) refuses it at save. + * + * The rulings this file pins (triage on objectui#11402): + * - new rows are seeded without a `field` key; + * - a measure with a blank Field box writes no `field`; + * - a dimension with a blank Field box shows as incomplete and is not saved + * with `''` — it is reported on the inspector's blocking-issue channel, the + * one the host's Save gate already reads (objectui#4527 / objectui#6900); + * - no `id` default is written on the author's behalf. + * + * The "saved" body below is the draft as the host holds it — patches merged + * shallowly, exactly as `ResourceEditPage` applies `onPatch` — sent through a + * JSON round trip, because the `dataset` type declares no `fromDraft` + * serialiser, so that draft IS the body the save sends. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react'; +import { DatasetSchema } from '@objectstack/spec/ui'; + +// Stub the catalog hooks so the inspector renders without a MetadataClient / +// network — the sibling suites' stubs, plus two catalog fields to pick from. +vi.mock('./useDatasetFields', () => ({ + useObjectOptions: () => ({ options: [], loading: false }), + useDatasetFieldCatalog: () => ({ + relationships: [], + fieldOptions: [ + { value: 'region', label: 'Region', type: 'text' }, + { value: 'amount', label: 'Amount', type: 'currency' }, + ], + loading: false, + }), + useDatasetUsage: () => ({ reports: 0, dashboards: 0, loading: false }), + fieldTypeToDimensionType: (t: string) => (t === 'date' ? 'date' : 'string'), +})); + +import { DatasetDefaultInspector, writeRowField } from './DatasetDefaultInspector'; + +afterEach(cleanup); + +type Draft = Record & { + dimensions?: Array>; + measures?: Array>; +}; + +const BASE: Draft = { name: 'sales', label: 'Sales', object: 'opportunity', dimensions: [], measures: [] }; + +/** A host that holds the draft and merges each patch the way `ResourceEditPage` does. */ +function mountHost(initial: Draft) { + const seen: { draft: Draft; blocking: number[] } = { draft: initial, blocking: [] }; + function Host() { + const [draft, setDraft] = React.useState(initial); + seen.draft = draft; + return ( + setDraft((d) => ({ ...d, ...patch }))} + onBlockingIssuesChange={(count) => seen.blocking.push(count)} + /> + ); + } + render(); + return seen; +} + +/** The body the save sends: the held draft through the JSON wire. */ +const savedBody = (draft: Draft): Draft => JSON.parse(JSON.stringify(draft)); + +async function pickSelectOption(label: string, option: string) { + fireEvent.keyDown(screen.getByRole('combobox', { name: label }), { key: 'ArrowDown' }); + await waitFor(() => expect(screen.queryAllByRole('option').length).toBeGreaterThan(0)); + fireEvent.click(screen.getByRole('option', { name: option })); +} + +async function pickComboOption(label: string, value: string) { + fireEvent.click(screen.getByRole('combobox', { name: label })); + await waitFor(() => expect(screen.queryAllByRole('option').length).toBeGreaterThan(0)); + const item = screen.getAllByRole('option').find((o) => o.textContent?.startsWith(value)); + expect(item, `the catalog option ${value}`).toBeTruthy(); + fireEvent.click(item!); +} + +describe('DatasetDefaultInspector — new rows carry no `field` key (objectui#11402)', () => { + it('seeds a new measure row with no `field` key', () => { + const onPatch = vi.fn(); + render(); + fireEvent.click(screen.getByText('Add measure')); + // `toStrictEqual`, not `toEqual`: a key present with the value `undefined` + // must fail here too — the row has to be built without it. + expect(onPatch.mock.calls[0][0].measures[0]).toStrictEqual({ name: '', aggregate: 'sum' }); + }); + + it('seeds a new dimension row with no `field` key — and no `id` default either', () => { + const onPatch = vi.fn(); + render(); + fireEvent.click(screen.getByText('Add dimension')); + expect(onPatch.mock.calls[0][0].dimensions[0]).toStrictEqual({ name: '', type: 'string' }); + }); +}); + +describe('DatasetDefaultInspector — a blank Field box on a measure writes no `field` (objectui#11402)', () => { + it('⭐ a measure added and saved with a blank Field box carries no `field`, and the saved dataset parses under DatasetSchema', async () => { + const seen = mountHost(BASE); + fireEvent.click(screen.getByText('Add measure')); + fireEvent.change(screen.getByPlaceholderText('e.g. revenue'), { target: { value: 'row_count' } }); + await pickSelectOption('Aggregate', 'count'); + + const held = seen.draft.measures![0]; + expect(Object.hasOwn(held, 'field'), 'the held row has no `field` key at all').toBe(false); + const body = savedBody(seen.draft); + expect(body.measures![0]).toStrictEqual({ name: 'row_count', aggregate: 'count' }); + + const parsed = DatasetSchema.safeParse(body); + expect(parsed.success, JSON.stringify(parsed.error?.issues)).toBe(true); + // Control: the same parse refuses a field-less NON-count measure, so the + // green above is a verdict the schema could have withheld. + const control = DatasetSchema.safeParse({ ...body, measures: [{ name: 'row_count', aggregate: 'sum' }] }); + expect(control.success).toBe(false); + }); + + it('a field-less measure is not held: only the dimension side is incomplete without a field', () => { + const seen = mountHost({ + ...BASE, + dimensions: [{ name: 'region', field: 'region', type: 'string' }], + measures: [{ name: 'row_count', aggregate: 'count' }], + }); + expect(seen.blocking.at(-1)).toBe(0); + }); + + it('a cleared Field value is written as an absent key, never as `\'\'`', () => { + // Typed as the inspector's own row shape: a field-less literal shares no + // key with `{ field?: string }`, so left to inference it reads as a weak + // type and `tsc -p tsconfig.test.json` refuses its other keys. + type Row = { name: string; aggregate: string; field?: string }; + const filled: Row = { name: 'n', aggregate: 'sum', field: 'amount' }; + const fieldless: Row = { name: 'n', aggregate: 'sum' }; + expect(writeRowField(filled, '')).toStrictEqual({ name: 'n', aggregate: 'sum' }); + expect(writeRowField(filled, ' ')).toStrictEqual({ name: 'n', aggregate: 'sum' }); + expect(writeRowField(fieldless, 'amount')).toStrictEqual({ name: 'n', aggregate: 'sum', field: 'amount' }); + }); +}); + +describe('DatasetDefaultInspector — a blank-Field dimension is held as incomplete (objectui#11402)', () => { + it('⭐ a dimension added with a blank Field box is reported to the Save gate until a field is picked', async () => { + const seen = mountHost(BASE); + expect(seen.blocking.at(-1), 'nothing to hold before the row exists').toBe(0); + + fireEvent.click(screen.getByText('Add dimension')); + expect(seen.blocking.at(-1), 'the field-less dimension is a blocking issue').toBe(1); + expect(Object.hasOwn(seen.draft.dimensions![0], 'field'), 'and it is not held as `field: \'\'`').toBe(false); + // Shown as incomplete: while the box is blank, the Field label carries the + // designer's required marker (objectui#10948) — the spec requires a + // dimension's field. + const markerOnFieldLabel = () => screen.getByText('Field').closest('label')?.querySelector('[data-required-marker="true"]'); + expect(markerOnFieldLabel(), 'the required marker on the blank Field label').toBeTruthy(); + + await pickComboOption('Field', 'region'); + expect(seen.draft.dimensions![0]).toMatchObject({ name: 'region', field: 'region', type: 'string' }); + expect(seen.blocking.at(-1), 'a picked field releases the hold').toBe(0); + expect(markerOnFieldLabel(), 'a complete row shows the plain label').toBeFalsy(); + }); + + it('the hold is never stricter than the spec: DatasetSchema refuses the field-less dimension it holds, and takes it once a field is picked', () => { + // The host keeps a verdict the server also returns advisory, because a + // client gate STRICTER than the server would wedge Save on a body the + // server takes (`ResourceEditPage`, the note above `blockingReport`). This + // hold is safe on that count only while the spec itself refuses a + // dimension without a `field` — if `DatasetDimensionSchema.field` ever + // turns optional, this goes red and the hold has to be revisited. (A + // stored `field: ''` is the one row it holds that this spec still takes: + // that one is ruled — not saved with `''` — and the Field box on screen + // repairs it, so it cannot wedge.) + const held = DatasetSchema.safeParse({ ...BASE, dimensions: [{ name: 'region', type: 'string' }] }); + expect(held.success).toBe(false); + expect(held.error?.issues.map((i) => i.path.join('.'))).toContain('dimensions.0.field'); + const picked = DatasetSchema.safeParse({ ...BASE, dimensions: [{ name: 'region', field: 'region', type: 'string' }] }); + expect(picked.success, JSON.stringify(picked.error?.issues)).toBe(true); + }); + + it('a stored dimension whose field is `\'\'` is held too, so it is not saved again as `\'\'`', () => { + const seen = mountHost({ ...BASE, dimensions: [{ name: 'region', field: '', type: 'string' }] }); + expect(seen.blocking.at(-1)).toBe(1); + }); + + it('counts each incomplete dimension once', () => { + const seen = mountHost({ + ...BASE, + dimensions: [ + { name: 'a', type: 'string' }, + { name: 'b', field: 'region', type: 'string' }, + { name: 'c', field: '', type: 'string' }, + ], + }); + expect(seen.blocking.at(-1)).toBe(2); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx index dcc8e5c420..725de67bac 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx @@ -27,6 +27,7 @@ import { InspectorTextField, InspectorSelectField, InspectorCheckboxField, + RequiredMarker, appendArray, spliceArray, } from './_shared.js'; @@ -139,6 +140,28 @@ type Measure = { filter?: FilterCondition; }; +/** + * objectui#11402 — a row's `field` has ONE spelling for "no field": the key is + * absent. `DatasetMeasureSchema.field` is optional (only `count` may omit it), + * and a dimension's `field` is required, so an empty string is never a value + * either kind can use — the analytics door answered 500 on the ObjectQL + * strategy for a `count` measure with `field: ''`, and the narrowed spec + * refuses `''` at save (objectstack-ai/objectstack#21240). + */ +function isBlankField(field: unknown): boolean { + return typeof field !== 'string' || field.trim() === ''; +} + +/** + * Write a row's `field`: a blank value removes the key rather than storing + * `''`. The row is rebuilt without the key — not given `field: undefined` — so + * the held draft and the body the save sends say the same thing. + */ +export function writeRowField(row: T, field: string): T { + const { field: _previous, ...rest } = row; + return (isBlankField(field) ? rest : { ...rest, field }) as T; +} + function SectionHeader({ title, count, onAdd, addLabel }: { title: string; count: number; onAdd?: () => void; addLabel: string }) { return (
@@ -347,7 +370,7 @@ export function objectChangePatch(next: string, current: string): Record (path.includes('.') ? path.split('.').pop() ?? path : path); + // The field itself goes through `writeRowField`, so a blank pick removes the + // key instead of storing `''` (objectui#11402). const pickDimensionField = (i: number, v: string) => { const opt = fieldOptions.find((o) => o.value === v); - const patch: Partial = opt?.type ? { field: v, type: fieldTypeToDimensionType(opt.type) } : { field: v }; - if (!dimensions[i]?.name) patch.name = leafName(v); // auto-name from field when unnamed - patchDimension(i, patch); + const patch: Partial = opt?.type ? { type: fieldTypeToDimensionType(opt.type) } : {}; + if (!isBlankField(v) && !dimensions[i]?.name) patch.name = leafName(v); // auto-name from field when unnamed + onPatch({ dimensions: dimensions.map((d, idx) => (idx === i ? writeRowField({ ...d, ...patch }, v) : d)) }); }; const pickMeasureField = (i: number, v: string) => { - const patch: Partial = { field: v }; - if (!measures[i]?.name) patch.name = leafName(v); // auto-name from field when unnamed - patchMeasure(i, patch); + const patch: Partial = {}; + if (!isBlankField(v) && !measures[i]?.name) patch.name = leafName(v); // auto-name from field when unnamed + onPatch({ measures: measures.map((m, idx) => (idx === i ? writeRowField({ ...m, ...patch }, v) : m)) }); }; + /* ─── A field-less dimension → the host's Save gate (objectui#11402) ─── + * + * A dimension's `field` is required (`DatasetDimensionSchema.field`), so a + * row whose Field box is blank is incomplete. It is reported on the + * blocking-issue channel the host already reads for this inspector family + * (objectui#4527 / objectui#6900), which holds Save, the autosave timer and + * the shortcut alike — so the row is never saved as `field: ''`, and never + * sent half-built. A stored `field: ''` counts too: it is the same blank box. + * While the box is blank, that row's Field label carries the designer's + * required marker (objectui#10948), so the row the hold is about is the one + * shown as incomplete; a row with a field shows the plain label. + * + * A measure is NOT counted: its `field` is optional, and whether its + * aggregate may go without one is the spec's verdict at save. + * + * The count is derived from the draft on every render — there is no queued + * verdict that could outlive the item it describes, so nothing is stamped + * here; the host's own stamp expires the count when this inspector goes. */ + const incompleteDimensions = dimensions.filter((d) => isBlankField(d.field)).length; + // Held in a ref so an unmemoized host callback cannot re-fire the effect. + const onBlockingIssuesChangeRef = React.useRef(onBlockingIssuesChange); + React.useEffect(() => { + onBlockingIssuesChangeRef.current = onBlockingIssuesChange; + }); + React.useEffect(() => { + onBlockingIssuesChangeRef.current?.(incompleteDimensions); + }, [incompleteDimensions]); + // One id prefix for every dimension row's Field label ⇄ combo pair; the row + // index keeps each pair distinct within this inspector. + const dimensionFieldIdPrefix = React.useId(); + return ( {}} hideClose> {datasetName && !usage.loading && ( @@ -548,7 +604,8 @@ export function DatasetDefaultInspector({ draft, onPatch, readOnly, name, locale title={tr('engine.inspector.dataset.dimensions')} count={dimensions.length} addLabel={tr('engine.inspector.dataset.addDimension')} - onAdd={readOnly ? undefined : () => onPatch({ dimensions: appendArray(dimensions, { name: '', field: '', type: 'string' }) })} + // No `field` key until one is picked (objectui#11402) — never `''`. + onAdd={readOnly ? undefined : () => onPatch({ dimensions: appendArray(dimensions, { name: '', type: 'string' }) })} /> {dimensions.map((d, i) => (
@@ -569,17 +626,26 @@ export function DatasetDefaultInspector({ draft, onPatch, readOnly, name, locale )}
patchDimension(i, { name: v })} placeholder={tr('engine.inspector.dataset.dimensionNamePlaceholder')} disabled={readOnly} mono /> - pickDimensionField(i, v)} - options={fieldComboOptions} - loading={catalogLoading} - placeholder={tr('engine.inspector.dataset.dimensionFieldPlaceholder')} - searchPlaceholder={tr('engine.form.searchFields')} - disabled={readOnly} - mono - /> + {/* The label is rendered here, not by the combo, so it can carry the + required marker while the box is blank (objectui#11402); `id` is + the combo's external-label variant. */} +
+ + pickDimensionField(i, v)} + options={fieldComboOptions} + loading={catalogLoading} + placeholder={tr('engine.inspector.dataset.dimensionFieldPlaceholder')} + searchPlaceholder={tr('engine.form.searchFields')} + disabled={readOnly} + mono + /> +
{(() => { const rel = missingRelationship(d.field, include); return rel ? onPatch({ include: appendArray(include, rel) })} /> : null; })()} patchDimension(i, { type: v })} disabled={readOnly} /> @@ -598,7 +664,8 @@ export function DatasetDefaultInspector({ draft, onPatch, readOnly, name, locale title={tr('engine.inspector.dataset.measures')} count={measures.length} addLabel={tr('engine.inspector.dataset.addMeasure')} - onAdd={readOnly ? undefined : () => onPatch({ measures: appendArray(measures, { name: '', aggregate: 'sum', field: '' }) })} + // No `field` key until one is picked (objectui#11402) — never `''`. + onAdd={readOnly ? undefined : () => onPatch({ measures: appendArray(measures, { name: '', aggregate: 'sum' }) })} /> {measures.map((m, i) => { const otherMeasures = measures.filter((_, idx) => idx !== i).map((x) => x.name).filter((n): n is string => !!n);