diff --git a/.changeset/wild-pugs-clap.md b/.changeset/wild-pugs-clap.md new file mode 100644 index 0000000000..7596b26293 --- /dev/null +++ b/.changeset/wild-pugs-clap.md @@ -0,0 +1,23 @@ +--- +'@objectstack/objectql': patch +--- + +写入载荷里的算子对象在标量字段上被响亮拒收(#5922) + +**行为变化**:此前静默入库的算子对象现在被拒绝。`update('task', { title: { $in: ['a','b'] } }, …)` 会抛 +`VALIDATION_FAILED`(字段码 `invalid_type`),而不再把 `{"$in":["a","b"]}` 原样交给驱动写进 `title` 列。 + +原本这条错误的命运取决于字段类型,而不取决于错误本身:`number` 会立刻响亮拒绝(`n must be a number`), +`text` 则零告警落库,之后以「这行的 title 变成了乱码」的形态在读路径上出现,离原因很远。实测(15 种字段类型, +记录型 driver 驱动真实引擎)显示放行的远不止 `text`:`textarea`、未声明 `options` 的 `select`、以及 +`lookup` 等引用类(ADR-0104 warn-first)同样放行;而 `select`(有 options)/ `url` / `email` / `phone` +之所以拒绝,只是因为 `String({ $in: […] })` 是 `"[object Object]"`,恰好过不了它们的正则或选项表 —— 一条 +在 4 种类型上偶然成立、在另外 11 种上不成立的规则,作者无法从元数据预测。 + +现在的规则只有一条:**声明值是标量的字段,一律不接受算子对象**。判定复用 spec 已导出的算子词表 +(`ALL_OPERATORS` + `RETIRED_FILTER_OPERATORS`),不是第六份手抄的 `startsWith('$')`,所以协议新增算子当天即 +自动收口。消息与 ADR-0104 的形状拒绝同族(同一 `invalid_value_shape` 文案,四语言均已本地化),点名字段、 +点名算子、点名声明类型。 + +刻意不动的两处:`json` 等结构化 JSON 类继续放行(`{ "$in": [...] }` 存在 `json` 列里是用户数据,不是写错的 +filter);多值字段保留既有的 `invalid_type_array` 拒绝。`insert` 与 `update`(单行与 multi)三个校验入口均已覆盖。 diff --git a/packages/objectql/src/validation/operator-object-write-value.test.ts b/packages/objectql/src/validation/operator-object-write-value.test.ts new file mode 100644 index 0000000000..bf48b5b816 --- /dev/null +++ b/packages/objectql/src/validation/operator-object-write-value.test.ts @@ -0,0 +1,281 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5922] A filter operator object is never a scalar field's VALUE. + * + * ## The asymmetry this closes + * + * Measured on `origin/main` @ `70f132c15` with a recording driver behind the + * real engine, writing one and the same `{ $in: ['a','b'] }` into fifteen + * declared field types: + * + * | type | before | after | + * |:--|:--|:--| + * | `text` / `textarea` | **ADMIT** — `driver.update` got `{"title":{"$in":["a","b"]}}` | refuse | + * | `select` with NO `options` | **ADMIT** | refuse | + * | `lookup` (and the reference class) | **ADMIT** + an ADR-0104 warn | refuse | + * | `number` / `percent` | refuse (`invalid_number`) | refuse | + * | `boolean` | refuse (`invalid_boolean`) | refuse | + * | `date` / `datetime` / `time` | refuse (`invalid_date` / …) | refuse | + * | `select` WITH `options`, `url`, `email`, `phone` | refuse — but only because `String({…})` is `"[object Object]"` | refuse | + * | `json` (structured-JSON class) | admit | **admit — by design** | + * + * The four "refuse" cells in the second-to-last row are the reason this rule + * had to be a rule rather than a patch on `text`: they were never deciding + * anything about operator objects, they were failing a regex on a stringified + * object. Eleven of fifteen types disagreed with the other four for no reason + * an author could predict from the metadata. + * + * ## What is deliberately NOT here + * + * - **`json` and the structured-JSON class** keep admitting. `{ "$in": [...] }` + * in a `json` column is a user's data; nothing distinguishes it from a + * mis-written filter and nothing should try. + * - **`data.id`.** Post-#5748/#5919 a non-scalar `data.id` is no longer bound + * as a primary key and rides into `driver.updateMany`'s SET payload instead — + * but `ENGINE_UPDATE_DISPATCH_CASES` rules that exact call ("operator object + * in data.id WITH multi:true — the declared bulk intent is honoured") and + * `engine-update-dispatch.test.ts` drives it against the real engine. That is + * a dispatch-axis contract; refusing it here would be a second answer to a + * question that module exists to answer once. Reported, filed, not patched. + */ + +import { describe, it, expect } from 'vitest'; +import { validateRecord, ValidationError } from './record-validator.js'; +import { ObjectQL } from '../engine.js'; + +const OP = { $in: ['a', 'b'] }; + +/** One field of each declared type the rule now closes. */ +const schema = { + fields: { + title: { type: 'text', label: 'Title' }, + note: { type: 'textarea', label: 'Note' }, + stage: { type: 'select', label: 'Stage', options: [{ value: 'a' }, { value: 'b' }] }, + stage_free: { type: 'select', label: 'Free stage' }, + due: { type: 'date', label: 'Due' }, + done: { type: 'boolean', label: 'Done' }, + owner: { type: 'lookup', label: 'Owner', reference: 'task' }, + n: { type: 'number', label: 'N' }, + payload: { type: 'json', label: 'Payload' }, + tags: { type: 'select', label: 'Tags', multiple: true, options: [{ value: 'a' }] }, + }, +}; + +function errorsOf(data: Record, mode: 'insert' | 'update' = 'update') { + try { + validateRecord(schema, data, mode); + } catch (e) { + if (e instanceof ValidationError) return e.fields; + throw e; + } + return []; +} + +describe('#5922 — an operator object is refused on every scalar-valued field', () => { + // The dispatch's four-type survey, plus the two string types the issue + // reported and the reference class the ADR-0104 branch used to wave through. + for (const field of ['title', 'note', 'stage', 'stage_free', 'due', 'done', 'owner', 'n'] as const) { + it(`refuses { ${field}: { $in: [...] } } and NAMES the operator`, () => { + const errs = errorsOf({ [field]: OP }); + expect(errs).toHaveLength(1); + expect(errs[0].field).toBe(field); + // The wire code stays ADR-0114's `invalid_type` — this is a shape + // refusal, not a new vocabulary entry. + expect(errs[0].code).toBe('invalid_type'); + // The message must name the FIELD, the offending OPERATOR and the + // DECLARED TYPE — the three facts `invalid_number` gives for its half of + // the same mistake, which is what makes the two answers one family. + expect(errs[0].message).toContain(schema.fields[field].label); + expect(errs[0].message).toContain('$in'); + expect(errs[0].message).toContain(schema.fields[field].type); + expect(errs[0].message).toMatch(/filter/i); + // …and the discrete constraint carries the same facts machine-readably. + expect(errs[0].constraint).toMatchObject({ type: schema.fields[field].type }); + }); + } + + it('names EVERY operator when the payload carries a compound filter', () => { + const errs = errorsOf({ title: { $gte: 1, $lte: 9 } }); + expect(errs).toHaveLength(1); + expect(errs[0].message).toContain('$gte'); + expect(errs[0].message).toContain('$lte'); + expect(errs[0].message).toContain('are filter operators'); + }); + + it('refuses a LOGICAL combinator too, not only field operators', () => { + // `$and`/`$or`/`$not` reach the same payload by the same accident. + expect(errorsOf({ title: { $or: [{ a: 1 }] } })).toHaveLength(1); + }); + + it('refuses a RETIRED operator — a filter spelled `$regex` is still a filter', () => { + // `$regex` is retired from the protocol but `plugin-auth` still emits it, + // so the shape is live. `RETIRED_FILTER_OPERATORS` is folded into the + // lookup set for exactly this. + expect(errorsOf({ title: { $regex: '^a' } })).toHaveLength(1); + }); + + it('applies on INSERT, not only on UPDATE', () => { + const errs = errorsOf({ title: OP }, 'insert'); + expect(errs.map((e) => e.field)).toContain('title'); + }); +}); + +describe('#5922 — what the rule deliberately leaves alone', () => { + it('admits an operator-shaped object in a `json` column (it is data, not a filter)', () => { + expect(() => validateRecord(schema, { payload: OP }, 'update')).not.toThrow(); + }); + + it('leaves a multi-value field on its existing `invalid_type_array` refusal', () => { + const errs = errorsOf({ tags: OP }); + expect(errs).toHaveLength(1); + expect(errs[0].message).toMatch(/array/i); + }); + + it('admits an UNDECLARED $-spelling — not a filter any backend executes', () => { + // The rule is derived from the spec's operator vocabulary, not from the + // `$` sigil. `{ $inn: … }` is out of scope by construction; judging every + // plain object on a scalar field is #5922's option A, which this is not. + expect(() => validateRecord(schema, { title: { $inn: ['a'] } }, 'update')).not.toThrow(); + }); + + it('admits a plain non-operator object (same reason)', () => { + expect(() => validateRecord(schema, { title: { a: 1 } }, 'update')).not.toThrow(); + }); +}); + +describe('#5922 — legal scalars still pass, and the pre-existing refusals still refuse', () => { + it('accepts a legal scalar for every closed type', () => { + expect(() => + validateRecord( + schema, + { + title: 'Ship it', note: 'a longer note', stage: 'a', stage_free: 'anything', + due: '2026-08-07', done: true, owner: 'rec_1', n: 42, payload: { any: 'shape' }, + tags: ['a'], + }, + 'insert', + ), + ).not.toThrow(); + }); + + it('a Date is a comparand, not an operator object', () => { + expect(() => validateRecord(schema, { due: new Date('2026-08-07') }, 'update')).not.toThrow(); + }); + + it('`number` keeps its own `invalid_number` refusal for a non-numeric SCALAR', () => { + // The regression pin the issue's premise rests on: the new rule sits in + // front of the type branches, so this asserts it did not swallow them. + const errs = errorsOf({ n: 'abc' }); + expect(errs).toHaveLength(1); + expect(errs[0].code).toBe('invalid_number'); + expect(errs[0].message).toBe('N must be a number'); + }); + + it('`boolean` and `select` keep their own refusals for non-operator garbage', () => { + expect(errorsOf({ done: 'perhaps' })[0].code).toBe('invalid_boolean'); + expect(errorsOf({ stage: 'zzz' })[0].code).toBe('invalid_option'); + }); +}); + +/** + * The end-to-end half: the issue's PROBE, run against the real engine. + * A unit test over `validateRecord` proves the rule; only this proves the rule + * is REACHED on the write path and that nothing dirty reaches storage. + */ +describe('#5922 — write path end to end: no dirty row reaches the driver', () => { + function recordingDriver() { + const rows = new Map>(); + const writes: Array<{ fn: string; data: unknown }> = []; + const driver: any = { + name: 'recording', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find() { return Array.from(rows.values()); }, + async findOne(_o: string, ast: any) { return rows.get(ast?.where?.id) ?? null; }, + async create(_o: string, data: Record) { + writes.push({ fn: 'create', data }); + const id = (data.id as string) ?? `rec_${rows.size + 1}`; + const row = { ...data, id }; + rows.set(id, row); + return row; + }, + async update(_o: string, id: string, data: Record) { + writes.push({ fn: 'update', data }); + const next = { ...(rows.get(id) ?? { id }), ...data, id }; + rows.set(id, next); + return next; + }, + async updateMany(_o: unknown, _ast: unknown, data: Record) { + writes.push({ fn: 'updateMany', data }); + return rows.size; + }, + async upsert(o: string, data: any) { return this.create(o, data); }, + async delete(_o: string, id: string) { return rows.delete(id); }, + async count() { return rows.size; }, + async bulkCreate(o: string, r: any[]) { return Promise.all(r.map((x) => this.create(o, x))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, rows, writes }; + } + + async function boot() { + const engine = new ObjectQL(); + const stub = recordingDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'op_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text', primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' }, + n: { name: 'n', label: 'N', type: 'number' }, + }, + } as any); + return { engine, ...stub }; + } + + it('refuses the issue’s exact update PROBE, and the driver is never touched', async () => { + const { engine, rows, writes } = await boot(); + rows.set('rec_1', { id: 'rec_1', title: 'orig' }); + writes.length = 0; + + await expect( + engine.update('op_task', { title: { $in: ['a', 'b'] } } as any, { where: { id: 'rec_1' } } as any), + ).rejects.toThrow(/filter operator/i); + + // The two halves of "no dirty data": the driver saw no write at all… + expect(writes).toEqual([]); + // …and the stored row still holds the value it had before the attempt. + expect(rows.get('rec_1')).toEqual({ id: 'rec_1', title: 'orig' }); + }); + + it('refuses it on insert too, and stores nothing', async () => { + const { engine, rows, writes } = await boot(); + await expect( + engine.insert('op_task', { title: { $in: ['a', 'b'] } } as any), + ).rejects.toThrow(/filter operator/i); + expect(writes).toEqual([]); + expect(rows.size).toBe(0); + }); + + it('refuses it on the MULTI update path (the second validateRecord call site)', async () => { + const { engine, rows, writes } = await boot(); + rows.set('rec_1', { id: 'rec_1', title: 'orig' }); + writes.length = 0; + await expect( + engine.update('op_task', { title: { $in: ['a', 'b'] } } as any, { multi: true } as any), + ).rejects.toThrow(/filter operator/i); + expect(writes).toEqual([]); + }); + + it('a legal write on the same path still lands', async () => { + const { engine, rows } = await boot(); + rows.set('rec_1', { id: 'rec_1', title: 'orig' }); + await engine.update('op_task', { title: 'renamed', n: 7 } as any, { where: { id: 'rec_1' } } as any); + expect(rows.get('rec_1')).toMatchObject({ title: 'renamed', n: 7 }); + }); +}); diff --git a/packages/objectql/src/validation/record-validator.ts b/packages/objectql/src/validation/record-validator.ts index 6f736b308d..98590fcdd5 100644 --- a/packages/objectql/src/validation/record-validator.ts +++ b/packages/objectql/src/validation/record-validator.ts @@ -11,6 +11,10 @@ * * Rules applied (in order, stop at first error per field): * + * - operator object a value carrying declared filter operators (`{ $in: […] }`) + * is refused on every field whose declared value is a SCALAR + * — a `where` clause pasted into the SET half of a write + * (#5922). * - `required` ADR-0113 write contract: on INSERT a missing/null/empty * value is rejected; on UPDATE a SUPPLIED missing value is * rejected (a PATCH may not null out a required field) while @@ -36,6 +40,9 @@ import { isMultiValueField as specIsMultiValueField, valueSchemaFor, + isPlainRecord, + ALL_OPERATORS, + RETIRED_FILTER_OPERATORS, REFERENCE_VALUE_TYPES, FILE_REFERENCE_TYPES, STRUCTURED_JSON_TYPES, @@ -280,6 +287,75 @@ function isMultiValueField(def: FieldDef): boolean { return specIsMultiValueField(def as { type: string; multiple?: boolean }); } +/** + * [#5922] Filter-operator keys, as a lookup set — **derived from the spec's own + * vocabulary, never restated here.** + * + * `ALL_OPERATORS` is `FILTER_OPERATORS` + `LOGICAL_OPERATORS`: the keys every + * backend is expected to evaluate, plus the three combinators. The retired ones + * (`$regex` / `$options`) are folded in from `RETIRED_FILTER_OPERATORS` because + * this rule asks *"was a filter written here?"*, and a filter carrying a retired + * spelling is still a filter — `plugin-auth`'s ObjectQL adapter emits `$regex` + * today, so the shape is live, not historical. + * + * ## Why derived and not `key.startsWith('$')` + * + * The repo already carries five hand-rolled `keys.some(k => k.startsWith('$'))` + * shape tests (`having-filter.ts`, `driver-memory`'s matcher and + * `filter-refusal.ts`, `driver-mongodb`'s `mongodb-filter.ts`, `driver-turso`'s + * `remote-transport.ts`). None of them is exported, and none is reachable from + * this package without inverting the layering — `@objectstack/objectql` depends + * on no driver. Writing a sixth `startsWith('$')` here is the accident #5659 + * names: one question, N private answers, and the day one of them changes only + * some of them follow. + * + * So this consumes the **shared vocabulary** instead, which is exactly what the + * two blessed consumers of `FILTER_OPERATORS` already do (`driver-memory`'s + * `SUPPORTED_FIELD_OPERATORS` and `service-analytics`' coverage test — the + * spec's own note calls deriving enforcement from it "the right design"). An + * operator added to the protocol is refused here the same day it is declared, + * with nothing to remember. + * + * The deliberate consequence: an **undeclared** `$`-spelling (`{ $inn: … }`) is + * NOT matched. It is not a filter any backend can execute, so it is not this + * rule's business — judging every plain object on a scalar field is the broader + * question (#5922's option A), and answering it here by accident would be a + * scope this ruling did not take. + */ +const FILTER_OPERATOR_KEYS: ReadonlySet = new Set([ + ...ALL_OPERATORS, + ...Object.keys(RETIRED_FILTER_OPERATORS), +]); + +/** + * [#5922] The declared filter operators this value carries as own keys — empty + * when the value is not an operator object at all. + * + * `isPlainRecord` (the spec's, shared with the authoring-key lint) is the + * object test: a `Date`, an array, a class instance and a `null` are all + * comparands or plain garbage, never a filter node. + */ +function filterOperatorKeysIn(value: unknown): string[] { + if (!isPlainRecord(value) || value instanceof Date) return []; + return Object.keys(value).filter((k) => FILTER_OPERATOR_KEYS.has(k)); +} + +/** + * [#5922] May this field's declared value legitimately BE an object? + * + * Two classes, and only two: a multi-value field stores an ARRAY (and is + * refused as `invalid_type_array` below when it is not one), and the + * structured-JSON class (`json` / `composite` / `repeater` / `record` / + * `location` / `address` / `vector`) stores an object BY DEFINITION — a `json` + * column holding `{ "$in": ["a","b"] }` is a user's data, not a mis-written + * filter, and this validator has no way to tell those apart nor any business + * trying. Every other declared type stores a SCALAR, which is what makes the + * operator-object rejection decidable at all. + */ +function valueMayBeAnObject(def: FieldDef): boolean { + return isMultiValueField(def) || STRUCTURED_JSON_TYPES.has(def.type); +} + /** * Coerce lone scalars into single-element arrays for multi-value fields, * IN PLACE, before validation (#2552). Legacy clients (e.g. pre-#2186 @@ -374,6 +450,55 @@ function validateOne( const t = def.type; + // ── [#5922] an operator object is a FILTER, never a scalar VALUE ─ + // `{ title: { $in: ['a','b'] } }` in a write payload is a `where` clause that + // was pasted into the SET half. Before this check the two halves of the same + // mistake had two fates chosen by the field's type: `number` said "n must be + // a number" and never reached the driver, while `text` handed the operator + // object to `driver.update` verbatim — the row then holds a serialized + // `{"$in":["a","b"]}` (or whatever the driver makes of it) and the damage + // surfaces far from its cause, as "this record's title turned into garbage". + // + // It runs BEFORE the per-type branches for two reasons. The near one: the + // ADR-0104 reference / structured-JSON branch below WARNS and reports the + // value to `onAdmittedValueShapeViolation` before returning null, and that + // sink means *this deployment stored a non-conforming value* — recording it + // for a write we are about to refuse would enter a counterexample against a + // contract nothing actually broke (see `AdmittedValueShapeViolation`). The + // far one: the branches that happen to refuse this shape today mostly do so + // by ACCIDENT — `select`/`url`/`email`/`phone` only because + // `String({ $in: [...] })` is `"[object Object]"`, which fails their regex or + // their option list. An accident that fires on four types and not on the + // other eleven is not a rule; this is. + // + // Reachability is external, not theoretical (#5922): flow's `update_record` + // spreads authored fields straight into `data` + // (`service-automation/src/builtin/crud-nodes.ts`), and a REST PATCH body is + // the same shape. An AI-authored filter builder emitting into `fields` + // instead of `filter` produces exactly this payload — PD #12's lenient + // consumer is precisely where such an error would otherwise hide. + if (!valueMayBeAnObject(def)) { + const ops = filterOperatorKeysIn(value); + if (ops.length > 0) { + const plural = ops.length > 1 ? 'are filter operators' : 'is a filter operator'; + return fail( + 'invalid_type', + { + type: t, + detail: + `${ops.join(', ')} ${plural}, not a value — a filter belongs in the query ` + + `'where', not in the write payload`, + }, + // Same wire code and same catalog entry as the ADR-0104 shape refusal + // one branch down: one sentence for "this value's SHAPE is wrong for + // this field's declared type", already localized in all four bundles. + // A fifth near-duplicate message key would be catalog drift, not + // clarity — the key is keyed by SENTENCE, and this is that sentence. + 'invalid_value_shape', + ); + } + } + // ── string types ──────────────────────────────────────────────── if (t === 'text' || t === 'textarea' || t === 'email' || t === 'url' || t === 'phone' || t === 'password' || t === 'markdown' || t === 'html' || t === 'richtext' || t === 'code') { const s = typeof value === 'string' ? value : String(value);