diff --git a/packages/fields/src/FieldEditWidget.test.ts b/packages/fields/src/FieldEditWidget.test.ts index ac5cb58ba6..1e2169594f 100644 --- a/packages/fields/src/FieldEditWidget.test.ts +++ b/packages/fields/src/FieldEditWidget.test.ts @@ -11,8 +11,27 @@ import { FORM_FIELD_TYPES, INLINE_EXCLUDED_FIELD_TYPES, hasFieldEditWidget, + mapFieldTypeToFormType, } from './index'; +/** + * A field type the form can render — either a direct widget-map key, or a spec + * spelling that resolves onto one through the alias table (`secret` → + * `field:password`, `autonumber` → `field:auto_number`, …). + * + * `FORM_FIELD_TYPES` alone is `Object.keys(fieldWidgetMap)`, which does NOT + * include the alias-only spellings — see the note at index.tsx's + * `resolveFormWidgetType`. Using it as the definition of "a real type" made the + * staleness check below reject `secret`, a genuine spec field type the form + * renders, purely because it arrives via an alias. + */ +function isRenderableFormType(t: string): boolean { + // `text` is the alias table's fallback, so a non-`field:text` result means an + // explicit entry exists. `text` itself is a direct widget key, so it is + // already covered by the first clause and no real type is missed here. + return FORM_FIELD_TYPES.includes(t) || mapFieldTypeToFormType(t) !== 'field:text'; +} + /** * Drift-guard: the inline cell editor reuses the form's field widgets, but the * two lists were hand-maintained separately and drifted — `lookup` (and other @@ -34,11 +53,21 @@ describe('inline editor ↔ form widget parity', () => { }); it('the exclusion set lists only real form types (no stale entries)', () => { - const known = new Set(FORM_FIELD_TYPES); - const stale = [...INLINE_EXCLUDED_FIELD_TYPES].filter((t) => !known.has(t)); + const stale = [...INLINE_EXCLUDED_FIELD_TYPES].filter((t) => !isRenderableFormType(t)); expect(stale).toEqual([]); }); + it('credential types are never inline-editable', () => { + // The grid's fallback for a type with no inline editor is a PLAIN TEXT input. + // Both of these are masked on read, so that input would show the mask as the + // value and write it straight back; `secret` also round-trips through an + // encrypted store (ADR-0100), so the cell holds an opaque ref, not the value. + for (const t of ['password', 'secret']) { + expect(hasFieldEditWidget(t), `${t} must not have an inline editor`).toBe(false); + expect(INLINE_EXCLUDED_FIELD_TYPES.has(t), `${t} must be explicitly excluded`).toBe(true); + } + }); + it('relational fields use the standard picker inline (regression: lookup was a text box)', () => { for (const t of ['lookup', 'master_detail', 'user', 'owner']) { expect(hasFieldEditWidget(t)).toBe(true); diff --git a/packages/fields/src/FieldEditWidget.tsx b/packages/fields/src/FieldEditWidget.tsx index 262d1ee02e..154ca14bc3 100644 --- a/packages/fields/src/FieldEditWidget.tsx +++ b/packages/fields/src/FieldEditWidget.tsx @@ -102,7 +102,13 @@ export const INLINE_EXCLUDED_FIELD_TYPES = new Set([ // Binary / attachment — edited from the record form, shown read-only in the grid. 'file', 'image', 'avatar', 'signature', // Heavy / full editors — better in the record form than a cell. - 'markdown', 'html', 'richtext', 'password', + 'markdown', 'html', 'richtext', + // Credentials — never inline-editable. Both are masked on read, so the cell has + // no real value to seed an editor with, and the grid's fallback is a PLAIN TEXT + // input: it would render the mask as if it were the value and write it straight + // back. `secret` additionally round-trips through an encrypted store (ADR-0100, + // data/field.zod.ts) — the cell holds an opaque ref, not the secret. + 'password', 'secret', // Containers / non-authorable — a sub-form / sub-grid / embedding vector // doesn't belong in a single cell. 'object', 'grid', 'vector', diff --git a/packages/fields/src/widgets/FilterConditionField.tsx b/packages/fields/src/widgets/FilterConditionField.tsx index 7af3003056..ba25d8c701 100644 --- a/packages/fields/src/widgets/FilterConditionField.tsx +++ b/packages/fields/src/widgets/FilterConditionField.tsx @@ -102,7 +102,13 @@ function toArray(value: any): any[] { return value == null ? [] : [value]; } -function condToMongo(c: BuilderCondition, typeOf: (f: string) => string | undefined): Record | null { +/** + * Builder condition → `$`-operator criteria. Exported for tests: this is the + * chokepoint where a builder token becomes a spec `FieldOperatorsSchema` key, + * and a wrong spelling here is rejected downstream by `convertFiltersToAST` + * rather than at authoring time. @internal + */ +export function condToMongo(c: BuilderCondition, typeOf: (f: string) => string | undefined): Record | null { const { field, operator, value } = c || ({} as BuilderCondition); if (!field) return null; const t = typeOf(field); @@ -111,7 +117,11 @@ function condToMongo(c: BuilderCondition, typeOf: (f: string) => string | undefi case 'equals': return { [field]: cv }; case 'notEquals': return { [field]: { $ne: cv } }; case 'contains': return { [field]: { $contains: value } }; - case 'notContains': return { [field]: { $ncontains: value } }; + // `$notContains` is the spec spelling (FieldOperatorsSchema, data/filter.zod.ts). + // This emitted `$ncontains` — a token that appears nowhere in @objectstack/spec and + // that convertFiltersToAST throws on, so every "does not contain" rule authored here + // was rejected downstream. See kvToCondition for reading the old spelling back. + case 'notContains': return { [field]: { $notContains: value } }; case 'isEmpty': return { [field]: { $in: [null, ''] } }; case 'isNotEmpty': return { [field]: { $nin: [null, ''] } }; case 'greaterThan': @@ -146,7 +156,13 @@ function arraysEqual(a: any, b: any[]): boolean { return Array.isArray(a) && a.length === b.length && a.every((v, i) => v === b[i]); } -function kvToCondition(field: string, v: any, idx: number): BuilderCondition | null { +/** + * Criteria → builder condition (the reverse of {@link condToMongo}). Returning + * `null` makes the builder refuse to load the rule ("criteria can't be + * represented"), so this must keep accepting spellings previously written. + * @internal + */ +export function kvToCondition(field: string, v: any, idx: number): BuilderCondition | null { const id = `c_${idx}_${field}`; if (v === null || typeof v !== 'object' || Array.isArray(v)) { return { id, field, operator: 'equals', value: v }; @@ -158,6 +174,10 @@ function kvToCondition(field: string, v: any, idx: number): BuilderCondition | n switch (op) { case '$ne': return { id, field, operator: 'notEquals', value: val }; case '$contains': return { id, field, operator: 'contains', value: val }; + // `$ncontains` is the pre-fix spelling this widget used to emit. Criteria saved + // before the fix still carry it, so keep reading it — dropping it here would make + // those rules fail to load ("criteria can't be represented") instead of migrating. + case '$notContains': case '$ncontains': return { id, field, operator: 'notContains', value: val }; case '$gt': return { id, field, operator: 'greaterThan', value: val }; case '$lt': return { id, field, operator: 'lessThan', value: val }; diff --git a/packages/fields/src/widgets/__tests__/FilterConditionField.operators.test.ts b/packages/fields/src/widgets/__tests__/FilterConditionField.operators.test.ts new file mode 100644 index 0000000000..e7429f3fd4 --- /dev/null +++ b/packages/fields/src/widgets/__tests__/FilterConditionField.operators.test.ts @@ -0,0 +1,100 @@ +/** + * 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. + */ + +/** + * Operator spelling guard for the sharing-rule criteria builder (#2901). + * + * `condToMongo` emitted `$ncontains` — a token that appears nowhere in + * `@objectstack/spec` and that objectui's own `convertFiltersToAST` throws on, + * naming the correct spelling (`$notContains`) in its error message. So every + * "does not contain" rule authored here validated in the UI and was then + * rejected downstream. Nothing caught it because the builder's operator list is + * hand-written and never compared against the spec vocabulary. + * + * These tests pin the emitted spellings against `FieldOperatorsSchema`'s keys + * and pin the round-trip, including the pre-fix spelling that stored criteria + * still carry. + */ +import { describe, it, expect } from 'vitest'; +import { condToMongo, kvToCondition } from '../FilterConditionField'; + +/** The `$`-prefixed operator keys of the spec's `FieldOperatorsSchema`. */ +const SPEC_OPERATORS = new Set([ + '$eq', '$ne', '$gt', '$gte', '$lt', '$lte', '$in', '$nin', + '$between', '$contains', '$notContains', '$startsWith', '$endsWith', + '$null', '$exists', +]); + +const noTypes = () => undefined; + +/** Pull the operator keys out of a `{ field: { $op: v } }` fragment. */ +function operatorsOf(frag: Record | null): string[] { + if (!frag) return []; + return Object.values(frag).flatMap((v) => + v !== null && typeof v === 'object' && !Array.isArray(v) ? Object.keys(v) : [], + ); +} + +describe('condToMongo emits only spec operator spellings', () => { + const builderOperators = [ + 'equals', 'notEquals', 'contains', 'notContains', 'isEmpty', 'isNotEmpty', + 'greaterThan', 'lessThan', 'greaterOrEqual', 'lessOrEqual', 'in', 'notIn', + 'before', 'after', + ]; + + it.each(builderOperators)('%s emits a spec-defined operator', (operator) => { + const frag = condToMongo( + { id: 'c1', field: 'name', operator, value: operator === 'in' || operator === 'notIn' ? ['a'] : 'a' } as any, + noTypes, + ); + const unknown = operatorsOf(frag).filter((op) => !SPEC_OPERATORS.has(op)); + expect(unknown, `${operator} emits an operator @objectstack/spec does not define`).toEqual([]); + }); + + it('notContains emits $notContains, not the pre-fix $ncontains', () => { + const frag = condToMongo({ id: 'c1', field: 'name', operator: 'notContains', value: 'x' } as any, noTypes); + expect(frag).toEqual({ name: { $notContains: 'x' } }); + }); + + it('between emits a range the spec defines', () => { + const frag = condToMongo({ id: 'c1', field: 'age', operator: 'between', value: [1, 5] } as any, noTypes); + expect(operatorsOf(frag).every((op) => SPEC_OPERATORS.has(op))).toBe(true); + }); +}); + +describe('kvToCondition round-trips what condToMongo writes', () => { + const cases: Array<[string, unknown]> = [ + ['notEquals', 'a'], + ['contains', 'a'], + ['notContains', 'a'], + ['greaterThan', 1], + ['lessThan', 1], + ['greaterOrEqual', 1], + ['lessOrEqual', 1], + ]; + + it.each(cases)('%s survives the round trip', (operator, value) => { + const frag = condToMongo({ id: 'c1', field: 'f', operator, value } as any, noTypes)!; + const [[field, v]] = Object.entries(frag); + expect(kvToCondition(field, v, 0)).toMatchObject({ field: 'f', operator }); + }); + + it('still reads the pre-fix $ncontains so stored criteria keep loading', () => { + // Dropping this would make rules saved before the fix fail to load entirely + // ("criteria can't be represented") rather than migrate. + expect(kvToCondition('name', { $ncontains: 'x' }, 0)).toMatchObject({ + field: 'name', + operator: 'notContains', + value: 'x', + }); + }); + + it('rejects an operator it cannot represent rather than guessing', () => { + expect(kvToCondition('name', { $nope: 'x' }, 0)).toBeNull(); + }); +});