Skip to content

Commit 24b62ee

Browse files
authored
fix(objectql): enforce array shape for multi-value fields in the write pipeline (#2552) (#2553)
1 parent 205d15b commit 24b62ee

5 files changed

Lines changed: 371 additions & 8 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
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`.
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect, vi, beforeEach } from 'vitest';
4+
import { ObjectQL } from './engine';
5+
import { SchemaRegistry } from './registry';
6+
7+
/**
8+
* #2552 — multi-value fields must reach the driver as ARRAYS.
9+
*
10+
* The write pipeline used to pass a lone scalar for a multiselect /
11+
* tags / select+multiple / lookup+multiple field straight through to the
12+
* driver (`PATCH { labels: "frontend" }` → stored verbatim as a string),
13+
* silently corrupting the column shape for every array-consumer. The
14+
* engine now normalizes unambiguous scalars into single-element arrays
15+
* BEFORE validation, and validation rejects any remaining non-array
16+
* shape with `invalid_type` instead of letting it hit storage.
17+
*
18+
* These tests assert on what the DRIVER receives — the actual corruption
19+
* point — not just on validator return values.
20+
*/
21+
vi.mock('./registry', () => {
22+
const instance: any = {
23+
getObject: vi.fn(),
24+
resolveObject: vi.fn((n: string) => instance.getObject(n)),
25+
registerObject: vi.fn(),
26+
getObjectOwner: vi.fn(),
27+
registerNamespace: vi.fn(),
28+
registerKind: vi.fn(),
29+
registerItem: vi.fn(),
30+
registerApp: vi.fn(),
31+
installPackage: vi.fn(),
32+
reset: vi.fn(),
33+
metadata: { get: vi.fn(() => new Map()) },
34+
};
35+
function SchemaRegistry() {
36+
return instance;
37+
}
38+
Object.assign(SchemaRegistry, instance);
39+
return {
40+
SchemaRegistry,
41+
computeFQN: (_ns: string | undefined, name: string) => name,
42+
parseFQN: (fqn: string) => ({ namespace: undefined, shortName: fqn }),
43+
RESERVED_NAMESPACES: new Set(['base', 'system']),
44+
};
45+
});
46+
47+
const PROJECT_SCHEMA = {
48+
name: 'project',
49+
fields: {
50+
name: { type: 'text' },
51+
labels: { type: 'multiselect', options: ['frontend', 'backend', 'design'] },
52+
tags: { type: 'tags' },
53+
channels: { type: 'select', multiple: true, options: ['email', 'sms'] },
54+
related_docs: { type: 'lookup', multiple: true, reference_to: 'document' },
55+
// Field.user expands to type 'user' at runtime — the showcase
56+
// team_members field ships exactly this shape (#2552 e2e regression).
57+
team_members: { type: 'user', multiple: true, reference: 'sys_user' },
58+
status: { type: 'select', options: ['active', 'done'] },
59+
},
60+
};
61+
62+
function makeDriver() {
63+
const created: any[] = [];
64+
const updated: any[] = [];
65+
const driver: any = {
66+
name: 'memory',
67+
supports: {},
68+
connect: vi.fn().mockResolvedValue(undefined),
69+
disconnect: vi.fn().mockResolvedValue(undefined),
70+
find: vi.fn().mockResolvedValue([]),
71+
findOne: vi.fn().mockResolvedValue({ id: 'r1' }),
72+
create: vi.fn(async (_obj: string, row: any) => {
73+
created.push(row);
74+
return { id: 'r1', ...row };
75+
}),
76+
update: vi.fn(async (_obj: string, id: string, row: any) => {
77+
updated.push(row);
78+
return { id, ...row };
79+
}),
80+
delete: vi.fn(),
81+
};
82+
return { driver, created, updated };
83+
}
84+
85+
async function makeEngine(driver: any) {
86+
vi.mocked((SchemaRegistry as any).getObject).mockImplementation((name: string) =>
87+
name === 'project' ? PROJECT_SCHEMA : undefined,
88+
);
89+
const ql = new ObjectQL();
90+
ql.registerDriver(driver, true);
91+
await ql.init();
92+
return ql;
93+
}
94+
95+
describe('engine write pipeline — multi-value scalar normalization (#2552)', () => {
96+
beforeEach(() => {
97+
vi.clearAllMocks();
98+
});
99+
100+
it('insert: wraps scalars so the driver receives arrays', async () => {
101+
const { driver, created } = makeDriver();
102+
const ql = await makeEngine(driver);
103+
await ql.insert('project', {
104+
name: 'P1',
105+
labels: 'frontend',
106+
tags: 'urgent',
107+
channels: 'email',
108+
related_docs: 'doc-1',
109+
team_members: 'user-1',
110+
});
111+
expect(created).toHaveLength(1);
112+
expect(created[0]).toMatchObject({
113+
labels: ['frontend'],
114+
tags: ['urgent'],
115+
channels: ['email'],
116+
related_docs: ['doc-1'],
117+
team_members: ['user-1'],
118+
});
119+
});
120+
121+
it('update: wraps scalars so the driver receives arrays', async () => {
122+
const { driver, updated } = makeDriver();
123+
const ql = await makeEngine(driver);
124+
await ql.update('project', { id: 'r1', labels: 'design', team_members: 'user-2' });
125+
expect(updated).toHaveLength(1);
126+
expect(updated[0]).toMatchObject({ labels: ['design'], team_members: ['user-2'] });
127+
});
128+
129+
it('update: legal arrays pass through untouched (select+multiple was mis-rejected before)', async () => {
130+
const { driver, updated } = makeDriver();
131+
const ql = await makeEngine(driver);
132+
await ql.update('project', {
133+
id: 'r1',
134+
labels: ['frontend', 'design'],
135+
channels: ['email', 'sms'],
136+
related_docs: ['d1', 'd2'],
137+
team_members: ['u1', 'u2'],
138+
});
139+
expect(updated[0]).toMatchObject({
140+
labels: ['frontend', 'design'],
141+
channels: ['email', 'sms'],
142+
related_docs: ['d1', 'd2'],
143+
team_members: ['u1', 'u2'],
144+
});
145+
});
146+
147+
it('update: un-wrappable junk is rejected BEFORE reaching the driver', async () => {
148+
const { driver, updated } = makeDriver();
149+
const ql = await makeEngine(driver);
150+
await expect(
151+
ql.update('project', { id: 'r1', labels: { nested: true } }),
152+
).rejects.toThrow(/invalid_type/i);
153+
expect(updated).toHaveLength(0);
154+
});
155+
156+
it('insert: invalid option inside a wrapped scalar still 400s', async () => {
157+
const { driver, created } = makeDriver();
158+
const ql = await makeEngine(driver);
159+
await expect(ql.insert('project', { name: 'P2', labels: 'nope' })).rejects.toThrow(
160+
/invalid_option/i,
161+
);
162+
expect(created).toHaveLength(0);
163+
});
164+
165+
it('single-value fields are untouched', async () => {
166+
const { driver, created } = makeDriver();
167+
const ql = await makeEngine(driver);
168+
await ql.insert('project', { name: 'P3', status: 'active' });
169+
expect(created[0].status).toBe('active');
170+
});
171+
});

packages/objectql/src/engine.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import { ExpressionEngine } from '@objectstack/formula';
3030
import type { Expression } from '@objectstack/spec';
3131
import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spec';
3232
import { bindHooksToEngine } from './hook-binder.js';
33-
import { validateRecord } from './validation/record-validator.js';
33+
import { validateRecord, normalizeMultiValueFields } from './validation/record-validator.js';
3434
import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields } from './validation/rule-validator.js';
3535
import { applyInMemoryAggregation } from './in-memory-aggregation.js';
3636

@@ -2129,6 +2129,7 @@ export class ObjectQL implements IDataEngine {
21292129
await this.encryptSecretFields(object, r, opCtx.context, hookContext.input.options);
21302130
}
21312131
for (const r of rows) {
2132+
normalizeMultiValueFields(schemaForValidation, r);
21322133
validateRecord(schemaForValidation, r, 'insert');
21332134
evaluateValidationRules(schemaForValidation as any, r, 'insert', { logger: this.logger });
21342135
}
@@ -2147,6 +2148,7 @@ export class ObjectQL implements IDataEngine {
21472148
);
21482149
await this.applyAutonumbers(object, row, opCtx.context, driverOwnsAutonumber);
21492150
await this.encryptSecretFields(object, row, opCtx.context, hookContext.input.options);
2151+
normalizeMultiValueFields(schemaForValidation, row);
21502152
validateRecord(schemaForValidation, row, 'insert');
21512153
evaluateValidationRules(schemaForValidation as any, row, 'insert', { logger: this.logger });
21522154
result = await driver.create(object, row, hookContext.input.options as any);
@@ -2263,6 +2265,7 @@ export class ObjectQL implements IDataEngine {
22632265
const updateSchema = this._registry.getObject(object);
22642266
if (hookContext.input.id) {
22652267
await this.encryptSecretFields(object, hookContext.input.data as Record<string, unknown>, opCtx.context, hookContext.input.options);
2268+
normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>);
22662269
validateRecord(updateSchema, hookContext.input.data as Record<string, unknown>, 'update');
22672270
if (needsPriorRecord(updateSchema as any) || (this.hooks.get('afterUpdate')?.length ?? 0) > 0) {
22682271
const priorAst: QueryAST = { object, where: { id: hookContext.input.id }, limit: 1 };
@@ -2276,6 +2279,7 @@ export class ObjectQL implements IDataEngine {
22762279
result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record<string, unknown>, hookContext.input.options as any);
22772280
} else if (options?.multi && driver.updateMany) {
22782281
await this.encryptSecretFields(object, hookContext.input.data as Record<string, unknown>, opCtx.context, hookContext.input.options);
2282+
normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>);
22792283
validateRecord(updateSchema, hookContext.input.data as Record<string, unknown>, 'update');
22802284
// Multi-row update: per-row prior state is not fetched (one query
22812285
// per matched row would be unbounded). state_machine /

packages/objectql/src/validation/record-validator.test.ts

Lines changed: 125 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import { describe, it, expect } from 'vitest';
4-
import { validateRecord } from './record-validator.js';
4+
import { validateRecord, normalizeMultiValueFields } from './record-validator.js';
55

66
/**
77
* Required-field validation, with the autonumber exemption (#1603).
@@ -65,3 +65,127 @@ describe('validateRecord — time field accepts time-of-day', () => {
6565
expect(() => validateRecord(ds, { d: 'not-a-date' }, 'insert')).toThrow(/invalid_date/i);
6666
});
6767
});
68+
69+
/**
70+
* Multi-value field shape enforcement + scalar normalization (#2552).
71+
*
72+
* A multiselect (and every other array-shaped field) used to accept a lone
73+
* scalar and store it VERBATIM — `PATCH { labels: "frontend" }` returned 200
74+
* and read back as a string, corrupting the column for every consumer that
75+
* expects an array (found via the console bulk-edit dialog, which pre-#2186
76+
* sent scalars for multi params). `select`+`multiple` was worse: a legal
77+
* ARRAY was stringified to "a,b" and rejected as invalid_option.
78+
*/
79+
describe('normalizeMultiValueFields — scalar → single-element array', () => {
80+
const schema = {
81+
fields: {
82+
labels: { type: 'multiselect', options: ['frontend', 'backend', 'design'] },
83+
tags: { type: 'tags' },
84+
channels: { type: 'select', multiple: true, options: ['email', 'sms'] },
85+
team_members: { type: 'lookup', multiple: true },
86+
// Field.user expands to type 'user' at runtime (NOT 'lookup') — the
87+
// showcase team_members regression that motivated widening the type set.
88+
watchers: { type: 'user', multiple: true, reference: 'sys_user' },
89+
attachments: { type: 'file', multiple: true },
90+
status: { type: 'select', options: ['active', 'done'] },
91+
owner: { type: 'lookup' },
92+
assignee: { type: 'user' },
93+
},
94+
};
95+
96+
it('wraps a scalar for multiselect / tags / select+multiple / lookup+multiple / user+multiple / file+multiple', () => {
97+
const data: Record<string, unknown> = {
98+
labels: 'frontend',
99+
tags: 'urgent',
100+
channels: 'email',
101+
team_members: 'user-1',
102+
watchers: 'user-2',
103+
attachments: 'file-key-1',
104+
};
105+
normalizeMultiValueFields(schema, data);
106+
expect(data).toEqual({
107+
labels: ['frontend'],
108+
tags: ['urgent'],
109+
channels: ['email'],
110+
team_members: ['user-1'],
111+
watchers: ['user-2'],
112+
attachments: ['file-key-1'],
113+
});
114+
});
115+
116+
it('leaves arrays, null/undefined, and single-value fields untouched', () => {
117+
const data: Record<string, unknown> = {
118+
labels: ['frontend', 'design'],
119+
tags: null,
120+
status: 'active',
121+
owner: 'user-1',
122+
assignee: 'user-2',
123+
};
124+
normalizeMultiValueFields(schema, data);
125+
expect(data).toEqual({
126+
labels: ['frontend', 'design'],
127+
tags: null,
128+
status: 'active',
129+
owner: 'user-1',
130+
assignee: 'user-2',
131+
});
132+
});
133+
134+
it('does NOT wrap non-scalar junk (left for validateRecord to reject)', () => {
135+
const data: Record<string, unknown> = { labels: { nested: true } };
136+
normalizeMultiValueFields(schema, data);
137+
expect(data.labels).toEqual({ nested: true });
138+
});
139+
});
140+
141+
describe('validateRecord — multi-value fields must be arrays', () => {
142+
const schema = {
143+
fields: {
144+
labels: { type: 'multiselect', options: ['frontend', 'backend'] },
145+
tags: { type: 'tags' },
146+
channels: { type: 'select', multiple: true, options: ['email', 'sms'] },
147+
team_members: { type: 'lookup', multiple: true },
148+
watchers: { type: 'user', multiple: true, reference: 'sys_user' },
149+
attachments: { type: 'file', multiple: true },
150+
status: { type: 'select', options: ['active', 'done'] },
151+
},
152+
};
153+
154+
it('rejects a raw (un-normalized) scalar with invalid_type', () => {
155+
for (const payload of [
156+
{ labels: 'frontend' },
157+
{ tags: 'urgent' },
158+
{ channels: 'email' },
159+
{ team_members: 'user-1' },
160+
{ watchers: 'user-1' },
161+
{ attachments: 'file-key-1' },
162+
]) {
163+
expect(() => validateRecord(schema, payload, 'update')).toThrow(/invalid_type/i);
164+
}
165+
});
166+
167+
it('rejects a plain-object shape with invalid_type', () => {
168+
expect(() => validateRecord(schema, { labels: { nested: true } }, 'update')).toThrow(/invalid_type/i);
169+
expect(() => validateRecord(schema, { team_members: { id: 'u1' } }, 'update')).toThrow(/invalid_type/i);
170+
});
171+
172+
it('accepts arrays (including for select+multiple, previously mis-rejected)', () => {
173+
expect(() =>
174+
validateRecord(
175+
schema,
176+
{ labels: ['frontend'], tags: ['a', 'b'], channels: ['email', 'sms'], team_members: ['u1', 'u2'], watchers: ['u1'], attachments: ['k1', 'k2'] },
177+
'update',
178+
),
179+
).not.toThrow();
180+
});
181+
182+
it('still validates array ELEMENTS against options', () => {
183+
expect(() => validateRecord(schema, { labels: ['nope'] }, 'update')).toThrow(/invalid_option/i);
184+
expect(() => validateRecord(schema, { channels: ['fax'] }, 'update')).toThrow(/invalid_option/i);
185+
});
186+
187+
it('does NOT regress single select / radio', () => {
188+
expect(() => validateRecord(schema, { status: 'active' }, 'update')).not.toThrow();
189+
expect(() => validateRecord(schema, { status: 'nope' }, 'update')).toThrow(/invalid_option/i);
190+
});
191+
});

0 commit comments

Comments
 (0)