From 35d739c2ff0cdd7c8690aed67c410531ae572992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Sat, 4 Jul 2026 09:28:13 -0700 Subject: [PATCH] fix(objectql): enforce array shape for multi-value fields in the write pipeline (#2552) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lone scalar sent at a multiselect / checkboxes / tags / select+multiple / lookup+multiple field used to pass validation and reach the driver verbatim (PATCH {"labels":"frontend"} → 200, stored as a string), silently corrupting the column shape for every array-consumer. select+multiple was worse: a legal ARRAY was stringified to "a,b" and mis-rejected as invalid_option, while lookup+multiple had no shape check at all. - normalizeMultiValueFields(): wrap unambiguous scalars (string/number/boolean) into single-element arrays, in place, before validation — called at all four validateRecord sites in engine insert/update (covers REST, engine callers, and SeedLoader, which all funnel through them) - validateOne(): multi-value fields must be arrays; anything un-wrappable (plain objects, nested junk) is rejected with a new `invalid_type` code instead of hitting storage - select/radio + multiple:true now routes through the multi-value branch with per-element option validation Closes #2552 Co-Authored-By: Claude --- .changeset/multivalue-field-shape.md | 5 + .../src/engine-multivalue-normalize.test.ts | 171 ++++++++++++++++++ packages/objectql/src/engine.ts | 6 +- .../src/validation/record-validator.test.ts | 126 ++++++++++++- .../src/validation/record-validator.ts | 71 +++++++- 5 files changed, 371 insertions(+), 8 deletions(-) create mode 100644 .changeset/multivalue-field-shape.md create mode 100644 packages/objectql/src/engine-multivalue-normalize.test.ts diff --git a/.changeset/multivalue-field-shape.md b/.changeset/multivalue-field-shape.md new file mode 100644 index 0000000000..4939fde3d4 --- /dev/null +++ b/.changeset/multivalue-field-shape.md @@ -0,0 +1,5 @@ +--- +"@objectstack/objectql": patch +--- + +Enforce array shape for multi-value fields in the write pipeline (#2552). Lone scalars sent at a `multiselect` / `checkboxes` / `tags` field — or at a `select` / `radio` / `lookup` / `user` / `file` / `image` field flagged `multiple: true` — are now normalized into single-element arrays before validation instead of being stored verbatim (which silently corrupted the column shape), un-wrappable shapes are rejected with a new `invalid_type` validation code, and a legal array at a `select`+`multiple` field is no longer mis-rejected as `invalid_option`. diff --git a/packages/objectql/src/engine-multivalue-normalize.test.ts b/packages/objectql/src/engine-multivalue-normalize.test.ts new file mode 100644 index 0000000000..aded294b3a --- /dev/null +++ b/packages/objectql/src/engine-multivalue-normalize.test.ts @@ -0,0 +1,171 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { ObjectQL } from './engine'; +import { SchemaRegistry } from './registry'; + +/** + * #2552 — multi-value fields must reach the driver as ARRAYS. + * + * The write pipeline used to pass a lone scalar for a multiselect / + * tags / select+multiple / lookup+multiple field straight through to the + * driver (`PATCH { labels: "frontend" }` → stored verbatim as a string), + * silently corrupting the column shape for every array-consumer. The + * engine now normalizes unambiguous scalars into single-element arrays + * BEFORE validation, and validation rejects any remaining non-array + * shape with `invalid_type` instead of letting it hit storage. + * + * These tests assert on what the DRIVER receives — the actual corruption + * point — not just on validator return values. + */ +vi.mock('./registry', () => { + const instance: any = { + getObject: vi.fn(), + resolveObject: vi.fn((n: string) => instance.getObject(n)), + registerObject: vi.fn(), + getObjectOwner: vi.fn(), + registerNamespace: vi.fn(), + registerKind: vi.fn(), + registerItem: vi.fn(), + registerApp: vi.fn(), + installPackage: vi.fn(), + reset: vi.fn(), + metadata: { get: vi.fn(() => new Map()) }, + }; + function SchemaRegistry() { + return instance; + } + Object.assign(SchemaRegistry, instance); + return { + SchemaRegistry, + computeFQN: (_ns: string | undefined, name: string) => name, + parseFQN: (fqn: string) => ({ namespace: undefined, shortName: fqn }), + RESERVED_NAMESPACES: new Set(['base', 'system']), + }; +}); + +const PROJECT_SCHEMA = { + name: 'project', + fields: { + name: { type: 'text' }, + labels: { type: 'multiselect', options: ['frontend', 'backend', 'design'] }, + tags: { type: 'tags' }, + channels: { type: 'select', multiple: true, options: ['email', 'sms'] }, + related_docs: { type: 'lookup', multiple: true, reference_to: 'document' }, + // Field.user expands to type 'user' at runtime — the showcase + // team_members field ships exactly this shape (#2552 e2e regression). + team_members: { type: 'user', multiple: true, reference: 'sys_user' }, + status: { type: 'select', options: ['active', 'done'] }, + }, +}; + +function makeDriver() { + const created: any[] = []; + const updated: any[] = []; + const driver: any = { + name: 'memory', + supports: {}, + connect: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn().mockResolvedValue({ id: 'r1' }), + create: vi.fn(async (_obj: string, row: any) => { + created.push(row); + return { id: 'r1', ...row }; + }), + update: vi.fn(async (_obj: string, id: string, row: any) => { + updated.push(row); + return { id, ...row }; + }), + delete: vi.fn(), + }; + return { driver, created, updated }; +} + +async function makeEngine(driver: any) { + vi.mocked((SchemaRegistry as any).getObject).mockImplementation((name: string) => + name === 'project' ? PROJECT_SCHEMA : undefined, + ); + const ql = new ObjectQL(); + ql.registerDriver(driver, true); + await ql.init(); + return ql; +} + +describe('engine write pipeline — multi-value scalar normalization (#2552)', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('insert: wraps scalars so the driver receives arrays', async () => { + const { driver, created } = makeDriver(); + const ql = await makeEngine(driver); + await ql.insert('project', { + name: 'P1', + labels: 'frontend', + tags: 'urgent', + channels: 'email', + related_docs: 'doc-1', + team_members: 'user-1', + }); + expect(created).toHaveLength(1); + expect(created[0]).toMatchObject({ + labels: ['frontend'], + tags: ['urgent'], + channels: ['email'], + related_docs: ['doc-1'], + team_members: ['user-1'], + }); + }); + + it('update: wraps scalars so the driver receives arrays', async () => { + const { driver, updated } = makeDriver(); + const ql = await makeEngine(driver); + await ql.update('project', { id: 'r1', labels: 'design', team_members: 'user-2' }); + expect(updated).toHaveLength(1); + expect(updated[0]).toMatchObject({ labels: ['design'], team_members: ['user-2'] }); + }); + + it('update: legal arrays pass through untouched (select+multiple was mis-rejected before)', async () => { + const { driver, updated } = makeDriver(); + const ql = await makeEngine(driver); + await ql.update('project', { + id: 'r1', + labels: ['frontend', 'design'], + channels: ['email', 'sms'], + related_docs: ['d1', 'd2'], + team_members: ['u1', 'u2'], + }); + expect(updated[0]).toMatchObject({ + labels: ['frontend', 'design'], + channels: ['email', 'sms'], + related_docs: ['d1', 'd2'], + team_members: ['u1', 'u2'], + }); + }); + + it('update: un-wrappable junk is rejected BEFORE reaching the driver', async () => { + const { driver, updated } = makeDriver(); + const ql = await makeEngine(driver); + await expect( + ql.update('project', { id: 'r1', labels: { nested: true } }), + ).rejects.toThrow(/invalid_type/i); + expect(updated).toHaveLength(0); + }); + + it('insert: invalid option inside a wrapped scalar still 400s', async () => { + const { driver, created } = makeDriver(); + const ql = await makeEngine(driver); + await expect(ql.insert('project', { name: 'P2', labels: 'nope' })).rejects.toThrow( + /invalid_option/i, + ); + expect(created).toHaveLength(0); + }); + + it('single-value fields are untouched', async () => { + const { driver, created } = makeDriver(); + const ql = await makeEngine(driver); + await ql.insert('project', { name: 'P3', status: 'active' }); + expect(created[0].status).toBe('active'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 570507337e..cc309ec825 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -30,7 +30,7 @@ import { ExpressionEngine } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spec'; import { bindHooksToEngine } from './hook-binder.js'; -import { validateRecord } from './validation/record-validator.js'; +import { validateRecord, normalizeMultiValueFields } from './validation/record-validator.js'; import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields } from './validation/rule-validator.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; @@ -2129,6 +2129,7 @@ export class ObjectQL implements IDataEngine { await this.encryptSecretFields(object, r, opCtx.context, hookContext.input.options); } for (const r of rows) { + normalizeMultiValueFields(schemaForValidation, r); validateRecord(schemaForValidation, r, 'insert'); evaluateValidationRules(schemaForValidation as any, r, 'insert', { logger: this.logger }); } @@ -2147,6 +2148,7 @@ export class ObjectQL implements IDataEngine { ); await this.applyAutonumbers(object, row, opCtx.context, driverOwnsAutonumber); await this.encryptSecretFields(object, row, opCtx.context, hookContext.input.options); + normalizeMultiValueFields(schemaForValidation, row); validateRecord(schemaForValidation, row, 'insert'); evaluateValidationRules(schemaForValidation as any, row, 'insert', { logger: this.logger }); result = await driver.create(object, row, hookContext.input.options as any); @@ -2263,6 +2265,7 @@ export class ObjectQL implements IDataEngine { const updateSchema = this._registry.getObject(object); if (hookContext.input.id) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); + normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); validateRecord(updateSchema, hookContext.input.data as Record, 'update'); if (needsPriorRecord(updateSchema as any) || (this.hooks.get('afterUpdate')?.length ?? 0) > 0) { const priorAst: QueryAST = { object, where: { id: hookContext.input.id }, limit: 1 }; @@ -2276,6 +2279,7 @@ export class ObjectQL implements IDataEngine { result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); } else if (options?.multi && driver.updateMany) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); + normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); validateRecord(updateSchema, hookContext.input.data as Record, 'update'); // Multi-row update: per-row prior state is not fetched (one query // per matched row would be unbounded). state_machine / diff --git a/packages/objectql/src/validation/record-validator.test.ts b/packages/objectql/src/validation/record-validator.test.ts index b1abb24d38..feecb8fe26 100644 --- a/packages/objectql/src/validation/record-validator.test.ts +++ b/packages/objectql/src/validation/record-validator.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import { validateRecord } from './record-validator.js'; +import { validateRecord, normalizeMultiValueFields } from './record-validator.js'; /** * Required-field validation, with the autonumber exemption (#1603). @@ -65,3 +65,127 @@ describe('validateRecord — time field accepts time-of-day', () => { expect(() => validateRecord(ds, { d: 'not-a-date' }, 'insert')).toThrow(/invalid_date/i); }); }); + +/** + * Multi-value field shape enforcement + scalar normalization (#2552). + * + * A multiselect (and every other array-shaped field) used to accept a lone + * scalar and store it VERBATIM — `PATCH { labels: "frontend" }` returned 200 + * and read back as a string, corrupting the column for every consumer that + * expects an array (found via the console bulk-edit dialog, which pre-#2186 + * sent scalars for multi params). `select`+`multiple` was worse: a legal + * ARRAY was stringified to "a,b" and rejected as invalid_option. + */ +describe('normalizeMultiValueFields — scalar → single-element array', () => { + const schema = { + fields: { + labels: { type: 'multiselect', options: ['frontend', 'backend', 'design'] }, + tags: { type: 'tags' }, + channels: { type: 'select', multiple: true, options: ['email', 'sms'] }, + team_members: { type: 'lookup', multiple: true }, + // Field.user expands to type 'user' at runtime (NOT 'lookup') — the + // showcase team_members regression that motivated widening the type set. + watchers: { type: 'user', multiple: true, reference: 'sys_user' }, + attachments: { type: 'file', multiple: true }, + status: { type: 'select', options: ['active', 'done'] }, + owner: { type: 'lookup' }, + assignee: { type: 'user' }, + }, + }; + + it('wraps a scalar for multiselect / tags / select+multiple / lookup+multiple / user+multiple / file+multiple', () => { + const data: Record = { + labels: 'frontend', + tags: 'urgent', + channels: 'email', + team_members: 'user-1', + watchers: 'user-2', + attachments: 'file-key-1', + }; + normalizeMultiValueFields(schema, data); + expect(data).toEqual({ + labels: ['frontend'], + tags: ['urgent'], + channels: ['email'], + team_members: ['user-1'], + watchers: ['user-2'], + attachments: ['file-key-1'], + }); + }); + + it('leaves arrays, null/undefined, and single-value fields untouched', () => { + const data: Record = { + labels: ['frontend', 'design'], + tags: null, + status: 'active', + owner: 'user-1', + assignee: 'user-2', + }; + normalizeMultiValueFields(schema, data); + expect(data).toEqual({ + labels: ['frontend', 'design'], + tags: null, + status: 'active', + owner: 'user-1', + assignee: 'user-2', + }); + }); + + it('does NOT wrap non-scalar junk (left for validateRecord to reject)', () => { + const data: Record = { labels: { nested: true } }; + normalizeMultiValueFields(schema, data); + expect(data.labels).toEqual({ nested: true }); + }); +}); + +describe('validateRecord — multi-value fields must be arrays', () => { + const schema = { + fields: { + labels: { type: 'multiselect', options: ['frontend', 'backend'] }, + tags: { type: 'tags' }, + channels: { type: 'select', multiple: true, options: ['email', 'sms'] }, + team_members: { type: 'lookup', multiple: true }, + watchers: { type: 'user', multiple: true, reference: 'sys_user' }, + attachments: { type: 'file', multiple: true }, + status: { type: 'select', options: ['active', 'done'] }, + }, + }; + + it('rejects a raw (un-normalized) scalar with invalid_type', () => { + for (const payload of [ + { labels: 'frontend' }, + { tags: 'urgent' }, + { channels: 'email' }, + { team_members: 'user-1' }, + { watchers: 'user-1' }, + { attachments: 'file-key-1' }, + ]) { + expect(() => validateRecord(schema, payload, 'update')).toThrow(/invalid_type/i); + } + }); + + it('rejects a plain-object shape with invalid_type', () => { + expect(() => validateRecord(schema, { labels: { nested: true } }, 'update')).toThrow(/invalid_type/i); + expect(() => validateRecord(schema, { team_members: { id: 'u1' } }, 'update')).toThrow(/invalid_type/i); + }); + + it('accepts arrays (including for select+multiple, previously mis-rejected)', () => { + expect(() => + validateRecord( + schema, + { labels: ['frontend'], tags: ['a', 'b'], channels: ['email', 'sms'], team_members: ['u1', 'u2'], watchers: ['u1'], attachments: ['k1', 'k2'] }, + 'update', + ), + ).not.toThrow(); + }); + + it('still validates array ELEMENTS against options', () => { + expect(() => validateRecord(schema, { labels: ['nope'] }, 'update')).toThrow(/invalid_option/i); + expect(() => validateRecord(schema, { channels: ['fax'] }, 'update')).toThrow(/invalid_option/i); + }); + + it('does NOT regress single select / radio', () => { + expect(() => validateRecord(schema, { status: 'active' }, 'update')).not.toThrow(); + expect(() => validateRecord(schema, { status: 'nope' }, 'update')).toThrow(/invalid_option/i); + }); +}); diff --git a/packages/objectql/src/validation/record-validator.ts b/packages/objectql/src/validation/record-validator.ts index c4d41ca8ee..646025fa4c 100644 --- a/packages/objectql/src/validation/record-validator.ts +++ b/packages/objectql/src/validation/record-validator.ts @@ -67,6 +67,7 @@ export interface FieldValidationError { | 'invalid_date' | 'invalid_time' | 'invalid_option' + | 'invalid_type' // Object-level validation rules (ADR-0020, see rule-validator.ts) | 'invalid_transition' | 'rule_violation' @@ -118,6 +119,50 @@ function optionValues(options: FieldDef['options']): string[] { ); } +/** + * A field whose persisted value is an ARRAY of scalars: either an + * inherently-multi type, or a single-value type flagged `multiple: true`. + * Per the spec (field.zod.ts), `multiple` applies to select/lookup/file/image; + * `radio` shares the select branch and `user` is stored identically to + * `lookup` (FK column, `multiple` ⇒ JSON array) — the runtime expands + * `Field.user` with `type: 'user'`, so it must be recognized here too. + */ +const MULTI_CAPABLE_TYPES = new Set(['select', 'radio', 'lookup', 'user', 'file', 'image']); + +function isMultiValueField(def: FieldDef): boolean { + const t = def.type; + if (t === 'multiselect' || t === 'checkboxes' || t === 'tags') return true; + return MULTI_CAPABLE_TYPES.has(t as string) && def.multiple === true; +} + +/** + * Coerce lone scalars into single-element arrays for multi-value fields, + * IN PLACE, before validation (#2552). Legacy clients (e.g. pre-#2186 + * console bulk-edit) PATCH `{ labels: "frontend" }` at a multiselect — + * without this the scalar used to be stored verbatim, silently corrupting + * the column's shape for every consumer that expects an array. + * + * Only unambiguous scalars (string/number/boolean) are wrapped; anything + * else (plain objects, nested garbage) is left untouched so that + * `validateRecord` can reject it with `invalid_type`. + */ +export function normalizeMultiValueFields( + objectSchema: { fields?: Record } | undefined | null, + data: Record | undefined | null, +): void { + if (!objectSchema?.fields || !data) return; + for (const [name, value] of Object.entries(data)) { + if (SKIP_FIELDS.has(name) || isMissing(value)) continue; + const def = objectSchema.fields[name]; + if (!def || def.system || def.readonly || !isMultiValueField(def)) continue; + if (Array.isArray(value)) continue; + const t = typeof value; + if (t === 'string' || t === 'number' || t === 'boolean') { + data[name] = [value]; + } + } +} + function validateOne(name: string, def: FieldDef, value: unknown): FieldValidationError | null { // ── required ──────────────────────────────────────────────────── // `autonumber` is runtime-owned: the value is generated by the engine / @@ -199,19 +244,33 @@ function validateOne(name: string, def: FieldDef, value: unknown): FieldValidati return { field: name, code: 'invalid_time', message: `${name} must be a valid time (HH:MM or HH:MM:SS)` }; } - // ── select / multiselect / radio ──────────────────────────────── - if (t === 'select' || t === 'radio') { + // ── select / radio (single-value) ─────────────────────────────── + // A `select`/`radio` flagged `multiple: true` is a multiselect in + // disguise — it falls through to the multi-value branch below (#2552; + // previously an array here was stringified to "a,b" and wrongly + // rejected as invalid_option, while a scalar slipped straight through). + if ((t === 'select' || t === 'radio') && def.multiple !== true) { const allowed = optionValues(def.options); if (allowed.length > 0 && !allowed.includes(String(value))) { return { field: name, code: 'invalid_option', message: `${name} must be one of: ${allowed.join(', ')}`, options: allowed }; } return null; } - if (t === 'multiselect' || t === 'checkboxes' || t === 'tags') { + + // ── multi-value fields: value must be an ARRAY ────────────────── + // Scalars are wrapped upstream by `normalizeMultiValueFields`; whatever + // still isn't an array here (objects, nested junk) is a shape error — + // storing it verbatim corrupts the column for every array-consumer (#2552). + if (isMultiValueField(def)) { + if (!Array.isArray(value)) { + return { field: name, code: 'invalid_type', message: `${name} must be an array of values` }; + } + // Reference / attachment types carry IDs or storage keys, not options — + // reference integrity is handled elsewhere. + if (t === 'lookup' || t === 'user' || t === 'file' || t === 'image') return null; const allowed = optionValues(def.options); - if (allowed.length === 0) return null; - const arr = Array.isArray(value) ? value : [value]; - for (const v of arr) { + if (allowed.length === 0) return null; // free-form (tags without options) + for (const v of value) { if (!allowed.includes(String(v))) { return { field: name, code: 'invalid_option', message: `${name}: "${v}" is not one of: ${allowed.join(', ')}`, options: allowed }; }