Skip to content

Commit a8aa34c

Browse files
os-zhuangclaude
andauthored
fix(objectql): enforce validation rules, requiredWhen and option visibleWhen on multi-row updates (#3160)
* fix(objectql): enforce validation rules, requiredWhen and option visibleWhen on multi-row updates (#3106) The bulk branch of engine.update (options.multi → driver.updateMany) never called evaluateValidationRules, so every object-level rule (script, state_machine, format, cross_field, json_schema, conditional), field-level requiredWhen, and per-option visibleWhen check was a silent no-op there — the only signal was a warning that skipped format/json_schema-only schemas entirely. The engine now reads the row-scoped match set once (the same middleware- composed AST the write binds, shared with the #3042 readonlyWhen strip) and evaluates the stripped payload against each matched row's prior state; any error-severity violation throws ValidationError (annotated with the failing record id) before updateMany writes anything. Schemas needing no prior state are evaluated once against the payload with no fetch; rule-free schemas (messaging outboxes, settings rows) hit the evaluator's early return and are unaffected. Deliberate behavior change: bulk writes that previously slipped past declared rules now throw. Also corrects the two doc comments that overstated coverage (rule-validator.ts, validation.zod.ts) — both now name the remaining events:['delete'] gap, which is tracked separately — and refreshes the AGENTS.md PD #10 example that described the pre-fix state. Closes #3106 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCUSMJBsX14C3RQdWFw7N7 * docs(spec): regenerate validation reference for the updated module doc comment The validation.zod.ts module doc comment feeds the generated content/docs/references/data/validation.mdx; regenerate so `check:docs` passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCUSMJBsX14C3RQdWFw7N7 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 875b089 commit a8aa34c

8 files changed

Lines changed: 319 additions & 22 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@objectstack/objectql": patch
3+
"@objectstack/spec": patch
4+
---
5+
6+
Enforce validation rules, `requiredWhen`, and per-option `visibleWhen` on multi-row updates (#3106). The bulk branch of `engine.update` (`options.multi` → `driver.updateMany`) previously never called `evaluateValidationRules`, so every object-level rule (`script`, `state_machine`, `format`, `cross_field`, `json_schema`, `conditional`), field-level `requiredWhen`, and per-option `visibleWhen` check was a silent no-op there. The engine now reads the row-scoped match set (the same AST the write binds, one query shared with the `readonlyWhen` bulk strip) and evaluates the payload against each matched row's prior state; any error-severity violation rejects the whole batch with `ValidationError` (annotated with the failing record id) before anything is written. Schemas needing no prior state (`format`/`json_schema`-only) are evaluated once against the payload with no fetch, and rule-free schemas are unaffected. Behavior change: bulk writes that previously slipped past declared rules now throw. Doc comments in `rule-validator.ts` and `validation.zod.ts` no longer overstate coverage and name the remaining `events: ['delete']` gap (tracked separately).

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ Other scripts: `objectui:bump` (pull only), `objectui:build`, `objectui:clean`.
7171
- Pre-existing vars that don't fit (`OS_METADATA_WRITABLE`, `OS_EAGER_SCHEMAS`, `OS_SERVER_TIMING`) are **debt, not precedent** — new vars follow this rule; rename old ones via the deprecation helper below when touched.
7272

7373
When renaming a legacy var, use `readEnvWithDeprecation('OS_NEW', 'LEGACY')` from `@objectstack/types` (keeps legacy working one release). Third-party exceptions kept as-is: `NODE_ENV`, `HOME`, `OPENAI_API_KEY`, `TURSO_*`, OAuth `*_CLIENT_ID/SECRET`, `RESEND_API_KEY`, `POSTMARK_TOKEN`, `AI_GATEWAY_*`, `SMTP_*`. See #1382.
74-
10. **File issues for out-of-scope findings — don't silently expand scope or leave them buried.** When you hit a bug, gap, or unenforced capability that's unrelated to the current task, or too large to fix in scope, open a GitHub issue (`gh issue create`) with a clear repro/decision and link it from your PR. Corollary: **never advertise or demo a capability the runtime doesn't actually deliver** (declared ≠ enforced) — fix it, trim it, or file an issue, but don't fake coverage. Example: the spec once declared 9 validation-rule types while the write-path validator enforced only 3 (`state_machine`/`script`/`cross_field`); the gap was filed as #1475 rather than demoed in the showcase, then closed by **trimming** what could never be enforced (`unique`/`async`/`custom`) and **implementing** the rest — the spec now declares 6 and `rule-validator.ts` handles all 6. Note how narrow that claim stays even so: the evaluator is wired into insert and single-id update only, so a bulk `updateMany` still skips every rule — silently, and for `format`/`json_schema` without even the warning (#3106). A `case` label is not enforcement; check the **call site**.
74+
10. **File issues for out-of-scope findings — don't silently expand scope or leave them buried.** When you hit a bug, gap, or unenforced capability that's unrelated to the current task, or too large to fix in scope, open a GitHub issue (`gh issue create`) with a clear repro/decision and link it from your PR. Corollary: **never advertise or demo a capability the runtime doesn't actually deliver** (declared ≠ enforced) — fix it, trim it, or file an issue, but don't fake coverage. Example: the spec once declared 9 validation-rule types while the write-path validator enforced only 3 (`state_machine`/`script`/`cross_field`); the gap was filed as #1475 rather than demoed in the showcase, then closed by **trimming** what could never be enforced (`unique`/`async`/`custom`) and **implementing** the rest — the spec now declares 6 and `rule-validator.ts` handles all 6. Note how narrow that claim stayed even so: the evaluator was wired into insert and single-id update only, so a bulk `updateMany` silently skipped every rule — a second `declared ≠ enforced` gap one layer down, at the **call site** rather than the `switch`; filed as #3106 and closed by evaluating the bulk match set per row. A `case` label is not enforcement; check the **call site**.
7575
11. **Worktree-first — never edit on the shared `main` checkout.** This repo is edited by **multiple agents at once**; the shared `main` tree has its HEAD switched and reset *under you*, silently clobbering uncommitted work. Before your **first file edit**, you MUST be in a dedicated worktree on a feature branch: `git worktree add ../framework-<task> -b <branch> main && cd ../framework-<task> && pnpm install`. A PreToolUse hook (`.claude/hooks/guard-main-checkout.sh`) **enforces** this — it blocks `Edit`/`Write`/`NotebookEdit` unless the edited file is in a dedicated **worktree** — a feature branch on the *shared* checkout is **not** enough (it still gets switched under you) — and it checks the **edited file's own repo**, so sibling repos (`objectui`/`cloud`) you touch are covered too (override for a deliberate non-task fix with `OS_ALLOW_MAIN_EDITS=1`). Full playbook below.
7676
12. **Contract-first — fix the metadata, not the runtime.** This is a metadata-driven framework: `packages/spec` is the one contract between metadata *producers* and the runtime/renderers that *consume* it. When a piece of metadata "doesn't work," ask **first**: *is it spec-compliant? is this the long-term-correct direction?* If the metadata is wrong, fix it at the **producer** and **reject it at authoring/publish** (validation / lint) so the error surfaces loudly — do **not** add a lenient alias or `??` fallback in the consumer (a node executor, the REST layer, a renderer) to tolerate off-spec input. A tolerant fallback fossilizes the wrong convention into a second de-facto contract, dilutes the spec, and hides the producer's bug — one strict contract beats N dialects. This is an **internal** contract (we own both ends), so "be liberal in what you accept" (Postel) does **not** apply — that's for untrusted boundaries. Change the **spec** only when the spec itself is genuinely wrong, and then deliberately (edit the Zod schema + migrate), never by accreting consumer-side fallbacks. The existing `cfg.filter ?? cfg.filters` / `cfg.objectName ?? cfg.object` in the flow executors are **debt to pay down, not a pattern to copy**. *Worked example:* an AI-authored `create_record` used `fieldValues` / `today()` / `{{trigger.record.id}}` while the executor reads `fields` / `{TODAY()}` / `{record.id}` → the fix was correcting the authoring skill + a publish-gate lint that rejects the wrong shape (cloud#688), **not** a `cfg.fields ?? cfg.fieldValues` runtime alias (framework#2419, rejected). Strengthens #5.
7777

content/docs/references/data/validation.mdx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@ record** — it must be decidable from the incoming write (and, on update, the p
2121

2222
no I/O. Everything advertised here runs on the write path (see
2323

24-
`objectql/src/validation/rule-validator.ts`); nothing is a silent no-op.
24+
`objectql/src/validation/rule-validator.ts`) — insert, single-id update, and multi-row
25+
26+
(`multi: true`) update, where the evaluator runs once per matched row (#3106). One known gap:
27+
28+
rules declaring `events: ['delete']` are not yet evaluated on delete (tracked separately).
2529

2630
The system supports these validation types:
2731

packages/objectql/src/engine.test.ts

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -698,6 +698,150 @@ describe('ObjectQL Engine', () => {
698698
});
699699
});
700700

701+
describe('Bulk update validation enforcement (#3106)', () => {
702+
beforeEach(async () => {
703+
engine.registerDriver(mockDriver, true);
704+
await engine.init();
705+
(mockDriver as any).updateMany = vi.fn().mockResolvedValue(2);
706+
});
707+
708+
it('enforces a prior-free format rule on multi without fetching rows', async () => {
709+
// format / json_schema need nothing from the prior record, so the
710+
// bulk path must evaluate them against the payload with NO fetch.
711+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({
712+
name: 'acct',
713+
fields: { email: { type: 'text' } },
714+
validations: [{ type: 'format', name: 'email_format', message: 'email must be a valid email', field: 'email', format: 'email' }],
715+
} as any);
716+
717+
await expect(
718+
engine.update('acct', { email: 'not-an-email' }, { where: { status: 'active' }, multi: true } as any),
719+
).rejects.toThrow(/valid email/);
720+
expect((mockDriver as any).updateMany).not.toHaveBeenCalled();
721+
expect(mockDriver.find).not.toHaveBeenCalled();
722+
});
723+
724+
it('lets a valid payload through the same format rule', async () => {
725+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({
726+
name: 'acct',
727+
fields: { email: { type: 'text' } },
728+
validations: [{ type: 'format', name: 'email_format', message: 'email must be a valid email', field: 'email', format: 'email' }],
729+
} as any);
730+
731+
await engine.update('acct', { email: 'a@example.com' }, { where: { status: 'active' }, multi: true } as any);
732+
expect((mockDriver as any).updateMany).toHaveBeenCalledTimes(1);
733+
});
734+
735+
it('evaluates a state_machine rule per matched row and rejects the whole batch when one row violates', async () => {
736+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({
737+
name: 'task',
738+
fields: { status: { type: 'select' } },
739+
validations: [{ type: 'state_machine', name: 'status_flow', message: 'illegal status transition', field: 'status', transitions: { pending: ['in_flight'], done: ['archived'] } }],
740+
} as any);
741+
// Rows are fetched with the SAME row-scoped ast the write binds.
742+
vi.mocked(mockDriver.find).mockResolvedValue([
743+
{ id: 'a', status: 'pending' }, // pending → in_flight is legal
744+
{ id: 'b', status: 'done' }, // done → in_flight is not
745+
] as any);
746+
747+
await expect(
748+
engine.update('task', { status: 'in_flight' }, { where: { status: { $in: ['pending', 'done'] } }, multi: true } as any),
749+
).rejects.toThrow(/illegal status transition \(record b\)/);
750+
expect(mockDriver.find).toHaveBeenCalledTimes(1);
751+
const [obj, ast] = vi.mocked(mockDriver.find).mock.calls[0];
752+
expect(obj).toBe('task');
753+
expect((ast as any).where).toEqual({ status: { $in: ['pending', 'done'] } });
754+
expect((mockDriver as any).updateMany).not.toHaveBeenCalled();
755+
});
756+
757+
it('proceeds when every matched row passes the state_machine rule', async () => {
758+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({
759+
name: 'task',
760+
fields: { status: { type: 'select' } },
761+
validations: [{ type: 'state_machine', name: 'status_flow', message: 'illegal status transition', field: 'status', transitions: { pending: ['in_flight'], done: ['archived'] } }],
762+
} as any);
763+
vi.mocked(mockDriver.find).mockResolvedValue([
764+
{ id: 'a', status: 'pending' },
765+
{ id: 'b', status: 'pending' },
766+
] as any);
767+
768+
await engine.update('task', { status: 'in_flight' }, { where: { status: 'pending' }, multi: true } as any);
769+
expect((mockDriver as any).updateMany).toHaveBeenCalledTimes(1);
770+
});
771+
772+
it('does not fetch rows for a rule-free schema (outbox/settings bulk writes stay zero-cost)', async () => {
773+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any);
774+
775+
await engine.update('task', { status: 'done' }, { where: { status: 'pending' }, multi: true } as any);
776+
expect(mockDriver.find).not.toHaveBeenCalled();
777+
expect((mockDriver as any).updateMany).toHaveBeenCalledTimes(1);
778+
});
779+
780+
it('enforces requiredWhen against each matched row\'s merged record', async () => {
781+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({
782+
name: 'ticket',
783+
fields: {
784+
status: { type: 'select' },
785+
resolution: { type: 'text', requiredWhen: "record.status == 'closed'" },
786+
},
787+
} as any);
788+
vi.mocked(mockDriver.find).mockResolvedValue([
789+
{ id: 'a', status: 'open', resolution: 'fixed' }, // merged has a resolution
790+
{ id: 'b', status: 'open', resolution: null }, // merged is missing it
791+
] as any);
792+
793+
await expect(
794+
engine.update('ticket', { status: 'closed' }, { where: { status: 'open' }, multi: true } as any),
795+
).rejects.toThrow(/resolution is required \(record b\)/);
796+
expect((mockDriver as any).updateMany).not.toHaveBeenCalled();
797+
});
798+
799+
it('enforces per-option visibleWhen against each matched row (cascade gating)', async () => {
800+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({
801+
name: 'case',
802+
fields: {
803+
tier: { type: 'select' },
804+
priority: {
805+
type: 'select',
806+
options: [
807+
{ value: 'urgent', visibleWhen: "record.tier == 'gold'" },
808+
{ value: 'normal' },
809+
],
810+
},
811+
},
812+
} as any);
813+
vi.mocked(mockDriver.find).mockResolvedValue([
814+
{ id: 'a', tier: 'gold' }, // 'urgent' is available here
815+
{ id: 'b', tier: 'silver' }, // hidden option submitted for this row
816+
] as any);
817+
818+
await expect(
819+
engine.update('case', { priority: 'urgent' }, { where: { status: 'open' }, multi: true } as any),
820+
).rejects.toThrow(/option 'urgent' is not available \(record b\)/);
821+
expect((mockDriver as any).updateMany).not.toHaveBeenCalled();
822+
});
823+
824+
it('shares ONE row fetch between the readonlyWhen strip and rule evaluation', async () => {
825+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({
826+
name: 'invoice',
827+
fields: {
828+
amount: { type: 'number', readonlyWhen: 'record.locked == true' },
829+
},
830+
validations: [{ type: 'cross_field', name: 'amount_cap', message: 'amount exceeds limit', condition: 'record.amount > record.limit', fields: ['amount'] }],
831+
} as any);
832+
vi.mocked(mockDriver.find).mockResolvedValue([
833+
{ id: 'a', locked: false, amount: 10, limit: 100 },
834+
] as any);
835+
836+
await engine.update('invoice', { amount: 50 }, { where: { status: 'draft' }, multi: true } as any);
837+
expect(mockDriver.find).toHaveBeenCalledTimes(1);
838+
expect((mockDriver as any).updateMany).toHaveBeenCalledTimes(1);
839+
// The unlocked conditional field survives the strip.
840+
const [, , data] = (mockDriver as any).updateMany.mock.calls[0];
841+
expect(data).toHaveProperty('amount', 50);
842+
});
843+
});
844+
701845
describe('Expand Related Records', () => {
702846
beforeEach(async () => {
703847
engine.registerDriver(mockDriver, true);

0 commit comments

Comments
 (0)