diff --git a/.changeset/29176-optional-unique-blank-null.md b/.changeset/29176-optional-unique-blank-null.md new file mode 100644 index 00000000000..a530292ae2d --- /dev/null +++ b/.changeset/29176-optional-unique-blank-null.md @@ -0,0 +1,15 @@ +--- +'@objectstack/objectql': patch +--- + +fix(objectql): a cleared optional unique text field stores `null`, so a second record cleared is no longer refused `409 UNIQUE_VIOLATION` + +Clause-②: no +Metadata-protocol: none + +A cleared text input is sent as `''`, and the write path stored it as sent. Every unique constraint the drivers build is NULL-distinct, so `null` never collides, but `''` is a value: the first record whose SKU (tax id, employee number, …) was cleared saved, and the second was refused "A record with this value already exists" although neither held a value. Measured on SQLite and the memory driver: two `''` rows collide, two `null` rows do not. + +- **Now.** The engine's write door, which `insert`, `insertMany`, `update` (by id and by predicate) and the dry run (`validate`) apply before anything else reads the payload, stores exactly `''` as `null` on a column that is text-like (`text`, `textarea`, `email`, `url`, `phone`, `markdown`, `html`, `richtext`, `code`, `color`, `signature`, `qrcode`), not `required`, and covered by a unique constraint: the field's own `unique`, or membership in a declared `indexes[]` entry carrying `unique`. Every REST, batch, upsert and import door reaches the engine through those methods. +- **Unchanged.** A `required` field given `''` is refused `required` exactly as before, also when it declares a `defaultValue`. A real duplicate is still `409 UNIQUE_VIOLATION`. Whitespace is a value and is stored as sent. `password` and `secret` keep their `''` refusal. A column no unique constraint covers, and `select` / `lookup` columns, keep storing `''`. +- **Defaults.** An optional unique field that declares a `defaultValue` now takes it on insert when sent `''`, exactly as when sent `null`. +- **Rows already stored.** Existing `''` values are not migrated. They keep matching a `field = ''` filter; a newly cleared row matches `$null: true` instead. The `is_empty` filter (`$empty`) matches both, as before. A newly cleared row no longer collides with a stored `''`. diff --git a/packages/objectql/src/engine-optional-unique-blank-door.test.ts b/packages/objectql/src/engine-optional-unique-blank-door.test.ts new file mode 100644 index 00000000000..6de7be67329 --- /dev/null +++ b/packages/objectql/src/engine-optional-unique-blank-door.test.ts @@ -0,0 +1,259 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #29176 — an empty string on an OPTIONAL text-like column that a unique + * constraint covers reaches the driver as `null`, on every engine write door. + * + * ## The defect + * + * A cleared text input is sent as `''`, and the write path stored it verbatim. + * Every unique constraint the drivers build is NULL-distinct (driver-sql's + * `(organization_id, field)` index, the memory driver's key model), so `null` + * never collides — but `''` is a value, and the second record cleared, or + * created with `''`, was refused `409 UNIQUE_VIOLATION` although neither + * record holds a value. + * + * ## What this file pins + * + * What the DRIVER receives, door by door (the physical column on SQLite, and + * the 409 that no longer fires, are `packages/rest/src/rest-data-optional-unique-blank.test.ts`): + * + * - the rewrite, on both unique surfaces: a field's own `unique`, and a member + * of a declared `indexes[]` entry carrying `unique`; + * - its edges, each a control: a column no unique constraint covers keeps + * `''` (the spec's `str_empty` side of the line); whitespace is a value, as + * the `$empty` operator's ruled table reads it; `select` / `lookup` keep + * `''` (not text-like); a non-blank value is untouched; + * - `required` is unchanged: a required unique field's `''` is refused + * `required` exactly as before — including when the field declares a + * `defaultValue`, which a blank rewritten to `null` would have taken; + * - the credential ruling is unchanged: `''` on a `password` field is still + * refused, never rewritten into a clear. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { ValidationError } from './validation/record-validator.js'; +import { EmptyCredentialWriteError, EMPTY_CREDENTIAL_REFUSAL_CODE, EMPTY_CREDENTIAL_REFUSAL_STATUS } from './secret-fields.js'; + +interface Call { fn: string; data: Record } + +function makeStubDriver() { + const calls: Call[] = []; + const rows = new Map>(); + const idOf = (q: any): string | undefined => { + const w = q?.where ?? q?.filter ?? q; + const id = w?.id; + return typeof id === 'string' ? id : undefined; + }; + let n = 0; + const driver: any = { + name: 'stub', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + // Not a query-honouring `find`: nothing here reads rows back through it + // (the by-id prior read is `findOne`), so it answers the whole store. + async find() { return [...rows.values()]; }, + async findOne(_o: string, q: any) { + const id = idOf(q); + return (id ? rows.get(id) : rows.values().next().value) ?? null; + }, + async count() { return rows.size; }, + async create(_o: string, data: Record) { + calls.push({ fn: 'create', data: { ...data } }); + const row = { ...data, id: (data.id as string) ?? `r${++n}` }; + rows.set(row.id as string, row); + return row; + }, + async bulkCreate(_o: string, list: Record[]) { + calls.push({ fn: 'bulkCreate', data: { rows: list.map((r) => ({ ...r })) } }); + const out: Record[] = []; + for (const r of list) { + const row = { ...r, id: (r.id as string) ?? `r${++n}` }; + rows.set(row.id as string, row); + out.push(row); + } + return out; + }, + async update(_o: string, id: string, data: Record) { + calls.push({ fn: 'update', data: { ...data } }); + const row = { ...(rows.get(id) ?? {}), ...data, id }; + rows.set(id, row); + return row; + }, + async updateMany(_o: string, _ast: unknown, data: Record) { + calls.push({ fn: 'updateMany', data: { ...data } }); + return rows.size; + }, + async upsert(o: string, data: Record) { return this.create(o, data); }, + async delete() { return true; }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, calls, rows }; +} + +const OBJ = { + name: 'uniq_blank', + label: 'Unique blank', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + sku: { name: 'sku', type: 'text' as const, unique: true }, + tax_id: { name: 'tax_id', type: 'text' as const, unique: 'global' as const }, + email: { name: 'email', type: 'email' as const, unique: 'organization' as const }, + grp: { name: 'grp', type: 'text' as const }, + code: { name: 'code', type: 'text' as const }, + note: { name: 'note', type: 'text' as const }, + plain_idx: { name: 'plain_idx', type: 'text' as const }, + off: { name: 'off', type: 'text' as const, unique: false }, + pick: { name: 'pick', type: 'select' as const, unique: true, options: [{ value: 'a', label: 'A' }] }, + ref: { name: 'ref', type: 'lookup' as const, unique: true, reference: 'uniq_blank' }, + }, + indexes: [ + { name: 'idx_uniq_blank_grp_code', fields: ['grp', 'code'], unique: 'global' as const }, + { name: 'idx_uniq_blank_plain', fields: ['plain_idx'] }, + ], +}; + +const REQ = { + name: 'uniq_blank_req', + label: 'Unique blank required', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + sku: { name: 'sku', type: 'text' as const, unique: true, required: true }, + skud: { name: 'skud', type: 'text' as const, unique: true, required: true, defaultValue: 'D-1' }, + opt_d: { name: 'opt_d', type: 'text' as const, unique: true, defaultValue: 'O-1' }, + }, +}; + +const CRED = { + name: 'uniq_blank_cred', + label: 'Unique blank credential', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + pin: { name: 'pin', type: 'password' as const, unique: true }, + }, +}; + +/** The covered columns of {@link OBJ}: both unique surfaces, three scopes. */ +const COVERED_NAMES = ['sku', 'tax_id', 'email', 'code']; +/** The columns that keep their `''`: no unique constraint, or not text-like. */ +const KEPT_NAMES = ['note', 'plain_idx', 'off', 'pick', 'ref']; +const blanksOf = (names: readonly string[]) => Object.fromEntries(names.map((n) => [n, ''])); +const COVERED = blanksOf(COVERED_NAMES); +const KEPT = blanksOf(KEPT_NAMES); + +function expectCoveredNull(data: Record) { + for (const k of COVERED_NAMES) expect(data[k], k).toBeNull(); +} +function expectKeptBlank(data: Record) { + for (const k of KEPT_NAMES) expect(data[k], k).toBe(''); +} + +async function refusal(fn: () => Promise) { + try { await fn(); return null; } catch (e) { + return { + name: (e as Error).name, + code: (e as { code?: unknown }).code, + fields: e instanceof ValidationError ? e.fields.map((x) => [x.field, x.code]) : undefined, + }; + } +} + +describe('engine write doors: an optional unique text-like column stores "" as null (#29176)', () => { + let engine: ObjectQL; + let stub: ReturnType; + + beforeEach(async () => { + stub = makeStubDriver(); + engine = new ObjectQL(); + engine.registerDriver(stub.driver, true); + await engine.init(); + engine.registry.registerObject(OBJ as any); + engine.registry.registerObject(REQ as any); + engine.registry.registerObject(CRED as any); + }); + + const last = (fn: string) => [...stub.calls].reverse().find((c) => c.fn === fn)!.data; + const written = () => stub.calls + .filter((c) => c.fn === 'create' || c.fn === 'bulkCreate') + .flatMap((c) => (c.fn === 'bulkCreate' ? (c.data.rows as Record[]) : [c.data])); + + it('insert, one row: every covered column is null; the controls keep ""', async () => { + const payload: Record = { id: 'a', grp: 'g1', ...COVERED, ...KEPT }; + await engine.insert('uniq_blank', payload); + const sent = last('create'); + expectCoveredNull(sent); + expectKeptBlank(sent); + // The caller's own object is never rewritten. + expect(payload.sku).toBe(''); + }); + + it('insert, an array of rows and insertMany: every row', async () => { + await engine.insert('uniq_blank', [{ id: 'b1', ...COVERED }, { id: 'b2', ...COVERED }]); + const rows = written(); + expect(rows.map((r) => r.id)).toEqual(['b1', 'b2']); + for (const r of rows) expectCoveredNull(r); + + stub.calls.length = 0; + const outcomes = await engine.insertMany('uniq_blank', [{ id: 'm1', ...COVERED }]); + expect(outcomes.map((o) => o.ok)).toEqual([true]); + const many = written(); + expect(many).toHaveLength(1); + expectCoveredNull(many[0]!); + }); + + it('update by id and update by predicate', async () => { + await engine.insert('uniq_blank', { id: 'u1', sku: 'S1', tax_id: 'T1', email: 'a@b.co', grp: 'g', code: 'C1' }); + await engine.update('uniq_blank', { id: 'u1', ...COVERED, ...KEPT }); + const byId = last('update'); + expectCoveredNull(byId); + expectKeptBlank(byId); + + await engine.update('uniq_blank', { ...COVERED }, { where: { id: { $in: ['u1'] } }, multi: true } as any); + expectCoveredNull(last('updateMany')); + }); + + it('whitespace and every non-blank value pass through untouched', async () => { + await engine.insert('uniq_blank', { id: 'w1', sku: ' ', tax_id: ' x ', email: 'a@b.co', code: '0' }); + const sent = last('create'); + expect([sent.sku, sent.tax_id, sent.email, sent.code]).toEqual([' ', ' x ', 'a@b.co', '0']); + }); + + it('an optional covered field with a defaultValue: a blank takes it on insert, exactly as null does', async () => { + await engine.insert('uniq_blank_req', { id: 'd1', sku: 'S1', skud: 'K1', opt_d: '' }); + const blank = last('create'); + await engine.insert('uniq_blank_req', { id: 'd2', sku: 'S2', skud: 'K2', opt_d: null }); + const nul = last('create'); + expect([blank.opt_d, nul.opt_d]).toEqual(['O-1', 'O-1']); + }); + + it('required is unchanged: a required unique field refuses "" as `required` — insert, update and the dry run', async () => { + const expected = { name: 'ValidationError', code: 'VALIDATION_FAILED', fields: [['sku', 'required']] }; + expect(await refusal(() => engine.insert('uniq_blank_req', { id: 'q1', sku: '', skud: 'K1' }))).toEqual(expected); + + await engine.insert('uniq_blank_req', { id: 'q2', sku: 'S2', skud: 'K2' }); + expect(await refusal(() => engine.update('uniq_blank_req', { id: 'q2', sku: '' }))).toEqual(expected); + + const preview = await engine.validate('uniq_blank_req', { id: 'q3', sku: '', skud: 'K3' }); + expect(preview.valid).toBe(false); + expect(preview.results?.[0]?.errors.map((e: any) => [e.field, e.code])).toEqual([['sku', 'required']]); + }); + + it('required is unchanged: a required unique field with a defaultValue refuses "" — it is NOT defaulted', async () => { + // `applyFieldDefaults` reads `''` as a real value, so the default never + // fills it and `required` refuses it. Rewriting the blank to `null` here + // would let the default fill it and the write land. + expect(await refusal(() => engine.insert('uniq_blank_req', { id: 'r1', sku: 'S1', skud: '' }))) + .toEqual({ name: 'ValidationError', code: 'VALIDATION_FAILED', fields: [['skud', 'required']] }); + expect(written().some((r) => r.id === 'r1')).toBe(false); + }); + + it('the credential ruling is unchanged: "" on a unique password field is refused, never cleared', async () => { + let thrown: unknown; + try { await engine.insert('uniq_blank_cred', { id: 'p1', pin: '' }); } catch (e) { thrown = e; } + expect(thrown).toBeInstanceOf(EmptyCredentialWriteError); + expect(thrown).toMatchObject({ code: EMPTY_CREDENTIAL_REFUSAL_CODE, status: EMPTY_CREDENTIAL_REFUSAL_STATUS, field: 'pin' }); + expect(written().some((r) => r.id === 'p1')).toBe(false); + }); +}); diff --git a/packages/objectql/src/validation/record-validator.optional-unique-blank.test.ts b/packages/objectql/src/validation/record-validator.optional-unique-blank.test.ts new file mode 100644 index 00000000000..3afba818955 --- /dev/null +++ b/packages/objectql/src/validation/record-validator.optional-unique-blank.test.ts @@ -0,0 +1,99 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #29176 — the write door's one string-stored arm, at the validator module: + * `normalizeBlankTypedValues` stores `''` as `null` on an OPTIONAL text-like + * column that a unique constraint covers, and on no other string-stored + * column. + * + * The population is read from the spec's own `STRING_VALUE_TYPES`, so a type + * joining the text-like class tomorrow is covered here without an edit, and + * the two credential types are pinned OUT by name. The engine-level half (what + * reaches the driver on every door) is `../engine-optional-unique-blank-door.test.ts`; + * the physical column and the status, through REST on SQLite, are + * `packages/rest/src/rest-data-optional-unique-blank.test.ts`. + */ + +import { describe, it, expect } from 'vitest'; +import { STRING_VALUE_TYPES } from '@objectstack/spec/data'; +import { normalizeBlankTypedValues } from './record-validator.js'; + +const CREDENTIAL = ['password', 'secret']; +const textLike = [...STRING_VALUE_TYPES].filter((t) => !CREDENTIAL.includes(t)); +// Single-value string-stored types the `$empty` table reads `''` as a value on. +const otherStringStored = ['select', 'radio', 'lookup', 'master_detail', 'user', 'autonumber']; + +type Def = Record; +function schemaOf(types: readonly string[], extra: Def = {}, indexes?: unknown[]) { + const fields: Record = {}; + for (const t of types) { + fields[`f_${t}`] = { + name: `f_${t}`, + type: t, + ...(t === 'lookup' || t === 'master_detail' ? { reference: 'other' } : {}), + ...(t === 'select' || t === 'radio' ? { options: [{ value: 'a', label: 'A' }] } : {}), + ...extra, + }; + } + return { fields, ...(indexes ? { indexes } : {}) } as any; +} +const rowOf = (types: readonly string[], value: unknown) => Object.fromEntries(types.map((t) => [`f_${t}`, value])); + +describe('normalizeBlankTypedValues: optional unique text-like columns (#29176)', () => { + it('the population is the spec text-like class minus the credential types', () => { + // A control on the set itself: if it ever emptied, every case below would + // pass over nothing. + expect(textLike.sort()).toEqual( + ['code', 'color', 'email', 'html', 'markdown', 'phone', 'qrcode', 'richtext', 'signature', 'text', 'textarea', 'url'], + ); + for (const t of CREDENTIAL) expect(STRING_VALUE_TYPES.has(t), t).toBe(true); + }); + + it.each([[true], ['global'], ['organization']])('field-level unique %j: "" on every text-like column becomes null', (unique) => { + const out = normalizeBlankTypedValues(schemaOf(textLike, { unique }), rowOf(textLike, '')) as Record; + for (const t of textLike) expect(out[`f_${t}`], t).toBeNull(); + }); + + it('a member of a declared unique index becomes null; a plain index member keeps ""', () => { + const schema = schemaOf(['text'], {}, [{ fields: ['f_text', 'f_textarea'], unique: 'global' }]); + schema.fields.f_textarea = { name: 'f_textarea', type: 'textarea' }; + schema.fields.f_plain = { name: 'f_plain', type: 'text' }; + schema.indexes.push({ fields: ['f_plain'] }, { fields: ['f_plain'], unique: false }); + const out = normalizeBlankTypedValues(schema, { f_text: '', f_textarea: '', f_plain: '' }) as Record; + expect(out).toEqual({ f_text: null, f_textarea: null, f_plain: '' }); + }); + + it('the controls keep "": no unique constraint, unique: false, required, credentials, other string-stored types', () => { + const cases: Array<[string, any, Record]> = [ + ['no unique', schemaOf(textLike), rowOf(textLike, '')], + ['unique: false', schemaOf(textLike, { unique: false }), rowOf(textLike, '')], + ['required', schemaOf(textLike, { unique: true, required: true }), rowOf(textLike, '')], + ['credentials', schemaOf(CREDENTIAL, { unique: true }), rowOf(CREDENTIAL, '')], + ['other string-stored', schemaOf(otherStringStored, { unique: true }), rowOf(otherStringStored, '')], + ]; + for (const [label, schema, row] of cases) expect(normalizeBlankTypedValues(schema, row), label).toBe(row); + }); + + it('a lifecycle column the engine owns is never rewritten, even when declared unique', () => { + const schema = { fields: { id: { name: 'id', type: 'text', unique: true } } } as any; + const row = { id: '' }; + expect(normalizeBlankTypedValues(schema, row)).toBe(row); + }); + + it('whitespace, and every non-blank value, pass through untouched', () => { + for (const v of [' ', '\t', ' x ', 'abc', '0', null, undefined, 0, false]) { + const row = rowOf(textLike, v); + expect(normalizeBlankTypedValues(schemaOf(textLike, { unique: true }), row), String(v)).toBe(row); + } + }); + + it('pure: an array is rewritten per row, the caller rows are never mutated', () => { + const schema = schemaOf(['text'], { unique: true }); + const rows = [{ f_text: '' }, { f_text: 'S1' }]; + const out = normalizeBlankTypedValues(schema, rows) as Array>; + expect(out).not.toBe(rows); + expect(out).toEqual([{ f_text: null }, { f_text: 'S1' }]); + expect(out[1]).toBe(rows[1]); + expect(rows[0]!.f_text).toBe(''); + }); +}); diff --git a/packages/objectql/src/validation/record-validator.ts b/packages/objectql/src/validation/record-validator.ts index fa4da021e0e..b450ff13b98 100644 --- a/packages/objectql/src/validation/record-validator.ts +++ b/packages/objectql/src/validation/record-validator.ts @@ -100,6 +100,8 @@ import { NUMERIC_VALUE_TYPES, COMPUTED_VALUE_TYPES, NON_TEXT_STORED_VALUE_TYPES, + STRING_VALUE_TYPES, + isUniqueDeclared, percentScaleOf, parseNumericString, classifyFilterToken, @@ -302,6 +304,14 @@ interface FieldDef { * field whose list did not resolve as one that accepts no value at all. */ picklist?: string; + /** + * The field's unique declaration (`true` / `'global'` / `'organization'`), + * read only through the spec's `isUniqueDeclared` — the predicate every + * driver materializes its constraint by. [#29176] It decides whether the + * write door stores a text-like `''` as `null` + * ({@link normalizeBlankTypedValues}). + */ + unique?: unknown; } // [#24131] `isMissing` (the def-free "missing": `undefined`, `null`, a blank @@ -705,7 +715,9 @@ function isInReadonlyScope(name: string, def: FieldDef, scope: ReadonlyValueScop /** * [#20308] A BLANK string on a non-string-typed column is that column's typed - * blank, `null` — the write door's one reading of it. + * blank, `null` — the write door's one reading of it. [#29176] So is `''` on + * an optional text-like column a unique constraint covers (its own section + * below). * * "Blank" is exactly what {@link isMissing} calls missing: `''` or a string of * whitespace. "Non-string-typed" is the spec's `NON_TEXT_STORED_VALUE_TYPES` — @@ -722,14 +734,36 @@ function isInReadonlyScope(name: string, def: FieldDef, scope: ReadonlyValueScop * outcomes; objectui's date and time boxes send `''` when cleared. This makes * storage agree with the platform's own reading. * + * ## [#29176] The one string-stored arm: an optional column a unique constraint covers + * + * Exactly `''` on a column that is ALL of: text-like + * ({@link UNIQUE_BLANK_TEXT_TYPES}), not `required: true`, and covered by a + * unique constraint — its own `unique`, or membership in a declared + * `indexes[]` entry whose `unique` asks for one (`isUniqueDeclared`, the + * predicate the drivers build their constraints by) — is `null` from the door + * on. Every driver's unique constraint is NULL-distinct (driver-sql's index, + * the memory driver's key model), so a cleared value then holds no value to + * the index as well as to the reader; stored as `''` it was a value, and the + * second record cleared was refused `409 UNIQUE_VIOLATION` although neither + * held one. Its own edges, each deliberate: + * + * - exactly `''`, never whitespace: the `$empty` operator's ruled table reads + * `''` and `null` as one answer on a text-like column and whitespace as a + * value, so this rewrite changes no `$empty` / `is_empty` answer; + * - never a `required` column: there `''` is refused `required` as before, + * and must not first become a `null` a `defaultValue` would fill; + * - a blank here also takes a declared `defaultValue` on insert, as the typed + * arm's does — the door runs before the defaults, by design. + * * ## What it does NOT touch * - * ⛔ Every string-stored column — text, lookup ids, select values. There `''` - * is a value, and the spec's value round-trip fixture pins it (`str_empty`: - * an empty string must not become null). What "empty" means for those types is - * a separate question, not answered here. ⛔ A non-blank value, and every - * non-string value, of any type: `null`, `0`, `false`, `'abc'` pass through - * untouched, so every type check and refusal downstream sees what it saw. + * ⛔ Every other string-stored column — text no unique constraint covers, + * lookup ids, select values. There `''` is a value, and the spec's value + * round-trip fixture pins it (`str_empty`: an empty string must not become + * null). What "empty" means for those columns is a separate question, not + * answered here. ⛔ A non-blank value, and every non-string value, of any + * type: `null`, `0`, `false`, `'abc'` pass through untouched, so every type + * check and refusal downstream sees what it saw. * * ## Where it runs, and why there * @@ -752,35 +786,94 @@ function isInReadonlyScope(name: string, def: FieldDef, scope: ReadonlyValueScop * nothing changed, else a shallow copy (per row, and a copied array). */ export function normalizeBlankTypedValues( - objectSchema: { fields?: Record } | undefined | null, + objectSchema: { fields?: Record; indexes?: unknown } | undefined | null, data: T, ): T { const fields = objectSchema?.fields; if (!fields || !data || typeof data !== 'object') return data; + // Read once per call, and only when a row carries an `''` at all. + let uniqueBlank: ReadonlySet | undefined; + const uniqueBlankColumns = () => (uniqueBlank ??= optionalUniqueTextColumns(fields, objectSchema?.indexes)); if (Array.isArray(data)) { let rows: unknown[] | undefined; for (let i = 0; i < data.length; i++) { - const row = normalizeBlankTypedRow(fields, data[i]); + const row = normalizeBlankTypedRow(fields, uniqueBlankColumns, data[i]); if (row !== data[i]) (rows ??= data.slice())[i] = row; } return (rows ?? data) as T; } - return normalizeBlankTypedRow(fields, data) as T; + return normalizeBlankTypedRow(fields, uniqueBlankColumns, data) as T; } -function normalizeBlankTypedRow(fields: Record, row: unknown): unknown { +function normalizeBlankTypedRow( + fields: Record, + uniqueBlankColumns: () => ReadonlySet, + row: unknown, +): unknown { if (!isPlainRecord(row)) return row; let out: Record | undefined; for (const [name, value] of Object.entries(row)) { if (typeof value !== 'string' || !isMissing(value)) continue; // Own-property: a field name may be `constructor` / `valueOf`. const def = Object.prototype.hasOwnProperty.call(fields, name) ? fields[name] : undefined; - if (!def || !NON_TEXT_STORED_VALUE_TYPES.has(def.type)) continue; + if (!def) continue; + if (!NON_TEXT_STORED_VALUE_TYPES.has(def.type) && !(value === '' && uniqueBlankColumns().has(name))) continue; (out ??= { ...row })[name] = null; } return out ?? row; } +/** + * [#29176] The text-like types whose `''` the write door stores as `null` on + * an optional column a unique constraint covers ({@link normalizeBlankTypedValues}). + * + * The spec's `STRING_VALUE_TYPES` — exactly the "text-like" row of the + * `$empty` operator's ruled table, where `''` and `null` already answer as one + * — read as the constant, never re-listed, minus the two credential types. + * ⛔ `password` and `secret` stay out: a maintainer ruling refuses `''` on them + * (`EmptyCredentialWriteError`) while `null` means "clear", so rewriting the + * blank here would turn that refusal into a silent clear. + * + * ⛔ Not `select` / `radio` / `lookup` / `user` and the other single-value + * string-stored types: the same `$empty` table reads their `''` as a value, + * so rewriting it would change an `is_empty` answer. That is a separate + * question from this one. + */ +const UNIQUE_BLANK_TEXT_TYPES: ReadonlySet = new Set( + [...STRING_VALUE_TYPES].filter((t) => t !== 'password' && t !== 'secret'), +); + +/** + * [#29176] The columns of one object whose `''` the write door stores as + * `null`: text-like ({@link UNIQUE_BLANK_TEXT_TYPES}), not `required: true`, + * not a lifecycle column the engine owns, and covered by a unique constraint + * on either surface the drivers build one from — a field's own `unique`, or a + * declared `indexes[]` entry whose `unique` asks for one. Both surfaces are + * read through `isUniqueDeclared`, the drivers' own predicate, so a column + * the store does not constrain is never rewritten. + */ +function optionalUniqueTextColumns(fields: Record, indexes: unknown): ReadonlySet { + const covered = new Set(); + for (const [name, def] of Object.entries(fields)) { + if (def && isUniqueDeclared(def.unique)) covered.add(name); + } + if (Array.isArray(indexes)) { + for (const idx of indexes) { + if (!isPlainRecord(idx) || !isUniqueDeclared(idx.unique) || !Array.isArray(idx.fields)) continue; + for (const name of idx.fields) if (typeof name === 'string') covered.add(name); + } + } + const out = new Set(); + for (const name of covered) { + if (SKIP_FIELDS.has(name)) continue; + // Own-property: a field name may be `constructor` / `valueOf`. + const def = Object.prototype.hasOwnProperty.call(fields, name) ? fields[name] : undefined; + if (!def || def.required === true || !UNIQUE_BLANK_TEXT_TYPES.has(def.type)) continue; + out.add(name); + } + return out; +} + /** * [#20309] The declared types the record validator's number arm judges: the * spec's numeric class minus its server-computed class, both read as constants. diff --git a/packages/rest/src/rest-data-optional-unique-blank.test.ts b/packages/rest/src/rest-data-optional-unique-blank.test.ts new file mode 100644 index 00000000000..7dc747671c1 --- /dev/null +++ b/packages/rest/src/rest-data-optional-unique-blank.test.ts @@ -0,0 +1,202 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #29176 — a cleared optional unique text field, through the REST write doors, + * on a real engine (`ObjectQL` + sqlite `SqlDriver`) and the real `RestServer` + * routes (the harness `rest-data-blank-typed-value.test.ts` boots). + * + * The defect, measured on `origin/main` dedc3505 with this file: the first + * record cleared (or created with `''`) stored `''`, and the SECOND was + * refused `409 UNIQUE_VIOLATION` by the driver's unique index — `''` is a + * value to SQL `UNIQUE`, while `NULL` is distinct from every other `NULL`. The + * engine's write door now stores such a blank as `null`, so the index exempts + * it, on every door below. + * + * The PHYSICAL column is read with the driver's own query builder. Controls: a + * column no unique constraint covers stores `''` (the spec's `str_empty`); a + * real duplicate is still `409 UNIQUE_VIOLATION`; a required unique field's + * `''` is still `400 VALIDATION_FAILED` / `required`, with or without a + * `defaultValue`. Rows that ALREADY hold `''` are not migrated: they keep their + * value, a new clear no longer collides with them, and the `$empty` operator + * answers both. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server'; + +const OBJ = { + name: 'uniq_rest', label: 'Unique', systemFields: false, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const, required: true }, + sku: { name: 'sku', type: 'text' as const, label: 'SKU', unique: true }, + grp: { name: 'grp', type: 'text' as const }, + code: { name: 'code', type: 'text' as const }, + note: { name: 'note', type: 'text' as const }, + }, + indexes: [{ name: 'idx_uniq_rest_grp_code', fields: ['grp', 'code'], unique: 'global' as const }], +}; +const REQ = { + name: 'uniq_rest_req', label: 'Required unique', systemFields: false, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + sku: { name: 'sku', type: 'text' as const, unique: true, required: true }, + skud: { name: 'skud', type: 'text' as const, unique: true, required: true, defaultValue: 'D-1' }, + }, +}; + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +function createMockServer() { + const noop = () => {}; + return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; +} + +function makeRes() { + const res: any = { + write: () => true, end: () => {}, + header: () => res, + status: (code: number) => { res._status = code; return res; }, + json: (body: any) => { res._json = body; return res; }, + }; + return res; +} + +async function boot() { + const engine = new ObjectQL(); + liveEngines.push(engine); + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(OBJ as any); + engine.registry.registerObject(REQ as any); + await engine.syncSchemas(); + + const protocol = new ObjectStackProtocolImplementation(engine as any); + const rest = new RestServer(createMockServer() as any, protocol as any, { + api: { requireAuth: false }, batch: { enableBatchEndpoint: true }, + } as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const call = async (method: string, path: string, params: Record, body: unknown) => { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + expect(route, `${method} ${path}`).toBeDefined(); + const res = makeRes(); + await route!.handler({ params, body, query: {}, headers: {} } as any, res); + return { status: res._status ?? 200, body: res._json }; + }; + const knex = (driver as any).knex; + /** The physical row, read past every engine read coercion. */ + const stored = async (object: string, id: string) => + (await knex(object).where({ id }).first()) as Record | undefined; + return { engine, call, stored, knex }; +} + +const create = (ctx: Awaited>, body: Record, object = 'uniq_rest') => + ctx.call('POST', '/api/v1/data/:object', { object }, body); +const patch = (ctx: Awaited>, id: string, body: Record) => + ctx.call('PATCH', '/api/v1/data/:object/:id', { object: 'uniq_rest', id }, body); + +describe('REST write doors on SQLite: a cleared optional unique text field stores null (#29176)', () => { + let ctx: Awaited>; + beforeEach(async () => { ctx = await boot(); }); + + it('POST create with "" twice: both 201, both store NULL', async () => { + const a = await create(ctx, { id: 'c1', name: 'r1', sku: '' }); + const b = await create(ctx, { id: 'c2', name: 'r2', sku: '' }); + expect([a.status, b.status]).toEqual([201, 201]); + expect((await ctx.stored('uniq_rest', 'c1'))?.sku).toBeNull(); + expect((await ctx.stored('uniq_rest', 'c2'))?.sku).toBeNull(); + }); + + it('PATCH clearing the field on two records: both 200, both store NULL', async () => { + expect((await create(ctx, { id: 'u1', name: 'r1', sku: 'S1' })).status).toBe(201); + expect((await create(ctx, { id: 'u2', name: 'r2', sku: 'S2' })).status).toBe(201); + const first = await patch(ctx, 'u1', { sku: '' }); + const second = await patch(ctx, 'u2', { sku: '' }); + expect([first.status, second.status]).toEqual([200, 200]); + expect((await ctx.stored('uniq_rest', 'u1'))?.sku).toBeNull(); + expect((await ctx.stored('uniq_rest', 'u2'))?.sku).toBeNull(); + }); + + it('a member of a declared unique index: two rows with the same group and a cleared code both land', async () => { + const a = await create(ctx, { id: 'i1', name: 'r1', grp: 'g', code: '' }); + const b = await create(ctx, { id: 'i2', name: 'r2', grp: 'g', code: '' }); + expect([a.status, b.status]).toEqual([201, 201]); + expect((await ctx.stored('uniq_rest', 'i2'))?.code).toBeNull(); + }); + + it('batch create and createMany: every row lands, every blank stores NULL', async () => { + const bc = await ctx.call('POST', '/api/v1/data/:object/batch', { object: 'uniq_rest' }, { + operation: 'create', + records: [{ data: { id: 'b1', name: 'r1', sku: '' } }, { data: { id: 'b2', name: 'r2', sku: '' } }], + }); + expect(bc.status).toBe(200); + expect(bc.body.results.map((r: any) => r.success)).toEqual([true, true]); + + const cm = await ctx.call('POST', '/api/v1/data/:object/createMany', { object: 'uniq_rest' }, + [{ id: 'm1', name: 'r3', sku: '' }, { id: 'm2', name: 'r4', sku: '' }]); + expect(cm.status).toBe(201); + for (const id of ['b1', 'b2', 'm1', 'm2']) expect((await ctx.stored('uniq_rest', id))?.sku, id).toBeNull(); + }); + + it('a column no unique constraint covers keeps "" — the str_empty side', async () => { + expect((await create(ctx, { id: 'n1', name: 'r1', note: '' })).status).toBe(201); + expect((await ctx.stored('uniq_rest', 'n1'))?.note).toBe(''); + }); + + it('a real duplicate is still refused: 409 UNIQUE_VIOLATION, nothing stored', async () => { + expect((await create(ctx, { id: 'd1', name: 'r1', sku: 'S1' })).status).toBe(201); + const dup = await create(ctx, { id: 'd2', name: 'r2', sku: 'S1' }); + expect(dup.status).toBe(409); + expect(dup.body).toMatchObject({ code: 'UNIQUE_VIOLATION' }); + expect(await ctx.stored('uniq_rest', 'd2')).toBeUndefined(); + + // And by PATCH: clearing is not a licence to copy another row's value. + expect((await create(ctx, { id: 'd3', name: 'r3', sku: '' })).status).toBe(201); + const copy = await patch(ctx, 'd3', { sku: 'S1' }); + expect(copy.status).toBe(409); + expect(copy.body).toMatchObject({ code: 'UNIQUE_VIOLATION' }); + expect((await ctx.stored('uniq_rest', 'd3'))?.sku).toBeNull(); + }); + + it('a required unique field refuses "": 400 VALIDATION_FAILED / required, with or without a defaultValue', async () => { + const bare = await create(ctx, { id: 'q1', sku: '', skud: 'K1' }, 'uniq_rest_req'); + expect(bare.status).toBe(400); + expect(bare.body).toMatchObject({ code: 'VALIDATION_FAILED' }); + expect(bare.body.fields.map((x: any) => [x.field, x.code])).toEqual([['sku', 'required']]); + + // `''` is a supplied value to `applyFieldDefaults`, so the default never + // fills it: refused, exactly as before. + const defaulted = await create(ctx, { id: 'q2', sku: 'S2', skud: '' }, 'uniq_rest_req'); + expect(defaulted.status).toBe(400); + expect(defaulted.body.fields.map((x: any) => [x.field, x.code])).toEqual([['skud', 'required']]); + expect(await ctx.stored('uniq_rest_req', 'q1')).toBeUndefined(); + expect(await ctx.stored('uniq_rest_req', 'q2')).toBeUndefined(); + }); + + it('a row that already holds "" is not migrated, and a new clear no longer collides with it', async () => { + // Written past the engine, as the before-state stored it. + await ctx.knex('uniq_rest').insert({ id: 'legacy', name: 'old', sku: '' }); + const fresh = await create(ctx, { id: 'f1', name: 'new', sku: '' }); + expect(fresh.status).toBe(201); + expect((await ctx.stored('uniq_rest', 'legacy'))?.sku).toBe(''); + expect((await ctx.stored('uniq_rest', 'f1'))?.sku).toBeNull(); + + const ids = async (where: Record) => + ((await ctx.engine.find('uniq_rest', { where, fields: ['id'] })) as Array<{ id: string }>).map((r) => r.id).sort(); + // `= ''` answers the stored `''` only; `$null` the new NULL only; the + // ruled `$empty` (text-like: null or '') answers both, as it did before. + expect(await ids({ sku: '' })).toEqual(['legacy']); + expect(await ids({ sku: { $null: true } })).toEqual(['f1']); + expect(await ids({ sku: { $empty: true } })).toEqual(['f1', 'legacy']); + }); +});