From 5164c34b68312e223747b97493e40484c6601199 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 02:54:02 +0000 Subject: [PATCH] fix(objectql): fail closed on unevaluable validation predicates, and make the merged record total (#4649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A script/cross_field/conditional validation whose CEL predicate could not be evaluated was logged at WARN and SKIPPED, so the write went through. The rule stayed declared, listed in the metadata, and enforced nothing — on exactly the records whose shape triggered the fault. For a validation that inverts the guarantee: the rule exists to reject a write. Two halves, neither sufficient alone: 1. The record a predicate reads is TOTAL over the object's declared fields on UPDATE as well as insert — `null` when the key is in neither the payload nor the prior record — and the `previous` binding is materialised the same way. Without this, step 2 would 422 every legitimate predicate on any driver that stores only written columns (the shape hotcrm#630 reported). Materialisation covers DECLARED fields only, so a typo'd key stays unevaluable and reportable. 2. A predicate that still faults REJECTS the write, naming the rule and the offending key. `severity` still governs blocking, so an advisory rule stays advisory. A `conditional` now counts as needing the prior record whenever it declares a `when`: that predicate is evaluated against the merged record, so without the prior state it read a PATCH as though it were the whole record. Fail-closed evaluation immediately found two of our own example rules that had never enforced anything: `has(x)` is TRUE for a declared column holding NULL, so `has(a) && has(b) && a < b` faults on `null < null` on any driver that returns its NULL columns. Both are rewritten with `!= null` guards, and the rejection message now teaches that distinction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny --- .changeset/tender-donkeys-smoke.md | 54 +++ .../app-crm/src/objects/opportunity.object.ts | 6 +- .../src/data/objects/project.object.ts | 7 +- .../src/validation/rule-fail-closed.test.ts | 398 ++++++++++++++++++ .../src/validation/rule-validator.test.ts | 45 +- .../objectql/src/validation/rule-validator.ts | 243 +++++++++-- 6 files changed, 712 insertions(+), 41 deletions(-) create mode 100644 .changeset/tender-donkeys-smoke.md create mode 100644 packages/objectql/src/validation/rule-fail-closed.test.ts diff --git a/.changeset/tender-donkeys-smoke.md b/.changeset/tender-donkeys-smoke.md new file mode 100644 index 0000000000..d30019bc61 --- /dev/null +++ b/.changeset/tender-donkeys-smoke.md @@ -0,0 +1,54 @@ +--- +'@objectstack/objectql': minor +--- + +**Validation rules now fail CLOSED when their predicate cannot be evaluated, and the record a predicate reads is total over the object's declared fields (#4649).** + +⚠️ **Behaviour change — read this before upgrading.** A `script` / `cross_field` / +`conditional` validation whose CEL predicate faulted used to be logged at WARN and +**skipped**, so the write went through. The rule stayed declared, appeared in the +metadata and in any "what protects this object" listing, and enforced nothing. Two +changes close that, and they are load-bearing together: + +1. **The merged record is total on UPDATE, not just on INSERT.** Every field the object + declares is present when the predicate runs — `null` when it is in neither the payload + nor the prior record. Previously `previous` was whatever the driver returned, so on a + driver that stores only written columns a predicate referencing a declared column + aborted with `No such key` and the rule was skipped. The `previous` CEL binding is + materialised the same way. Insert and update now behave identically. +2. **A predicate that still cannot be evaluated rejects the write** with + `VALIDATION_FAILED`, naming the rule and — when the fault is a missing key — the key + the predicate read and how to fix it. A validation exists to reject a write; "the rule + could not be checked" must never resolve to "allowed". + +`severity` still governs blocking: an unevaluable `warning` / `info` rule is logged and +does not throw. + +**What you may see after upgrading** + +- **Rules that were never running start running.** A rule skipped because of a missing key + now evaluates and can reject writes it previously let through. This is not a regression — + it is the declaration finally being enforced — but on an existing deployment it can + surface as new `400 VALIDATION_FAILED` responses on writes that used to succeed. Review + each such rule: it is doing what its author wrote. +- **Predicates guarded with `has(...)` may now reject.** `has(x)` asks whether the key is + **present**, and a declared field holding `null` is present — so + `has(a) && has(b) && a < b` still faults on `null < null`. Such a rule never enforced + anything on rows with a null value (on any driver that returns its NULL columns); the + fault used to be swallowed and is now reported. **Guard with `!= null`, not `has(...)`:** + + ```diff + - condition: 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date' + + condition: 'record.start_date != null && record.end_date != null && record.end_date < record.start_date' + ``` + + The rejection message says this explicitly, and `error.fields[0].constraint` carries + `{ reason: 'unevaluable', missingKey?, hint?: 'null-comparison' }` for machine handling. + `has()` remains correct for asking whether an **undeclared** key exists. +- **A `conditional` rule now always fetches the prior record on update.** Its `when` is + evaluated against the merged record, so without the prior state it read a PATCH as if it + were the whole record. One extra `findOne` per update on objects that declare one. + +**Unchanged, deliberately:** a broken `regex` (`format`), an uncompilable JSON Schema +(`json_schema`), the field-level `requiredWhen` / `readonlyWhen` / option `visibleWhen` +predicates, and a rule that throws all keep their existing fail-open policy. diff --git a/examples/app-crm/src/objects/opportunity.object.ts b/examples/app-crm/src/objects/opportunity.object.ts index 4281f0413f..b952cf4832 100644 --- a/examples/app-crm/src/objects/opportunity.object.ts +++ b/examples/app-crm/src/objects/opportunity.object.ts @@ -109,7 +109,11 @@ export const Opportunity = ObjectSchema.create({ label: 'Close Date Must Be Future', description: 'Prevent back-dating the close_date of an OPEN opportunity. Closed (won/lost) deals legitimately carry a historical close date, so they are exempt.', fields: ['close_date'], - condition: P`has(record.close_date) && record.close_date < now() && record.stage != "closed_won" && record.stage != "closed_lost"`, + // `!= null`, not `has(...)` (#4649): `has(x)` is TRUE for a declared + // column holding NULL, so the old guard let `null < now()` fault and the + // rule silently did nothing on every opportunity created without a close + // date. + condition: P`record.close_date != null && record.close_date < now() && record.stage != "closed_won" && record.stage != "closed_lost"`, message: 'Close Date must be today or a future date.', events: ['insert'], }, diff --git a/examples/app-showcase/src/data/objects/project.object.ts b/examples/app-showcase/src/data/objects/project.object.ts index 72291538d3..bc42b96936 100644 --- a/examples/app-showcase/src/data/objects/project.object.ts +++ b/examples/app-showcase/src/data/objects/project.object.ts @@ -106,7 +106,12 @@ export const Project = ObjectSchema.create({ label: 'End After Start', description: 'Target end date must be on or after the start date.', fields: ['start_date', 'end_date'], - condition: P`has(record.start_date) && has(record.end_date) && record.end_date < record.start_date`, + // Guarded with `!= null`, NOT `has(...)` (#4649). `has(x)` asks whether + // the key is PRESENT — a declared column holding NULL is present, so + // `has(a) && has(b) && a < b` still faults on `null < null` and the rule + // enforced nothing on any project missing a date. It read as a guard and + // was not one. + condition: P`record.start_date != null && record.end_date != null && record.end_date < record.start_date`, message: 'Target End Date must be on or after the Start Date.', }, { diff --git a/packages/objectql/src/validation/rule-fail-closed.test.ts b/packages/objectql/src/validation/rule-fail-closed.test.ts new file mode 100644 index 0000000000..544d6ce756 --- /dev/null +++ b/packages/objectql/src/validation/rule-fail-closed.test.ts @@ -0,0 +1,398 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4649 — a script validation whose predicate could not be evaluated used to be + * SKIPPED with a WARN, so the rule was declared, listed in the metadata, and + * enforced nothing. For a validation that inverts the guarantee: the rule exists + * to reject a write, and its failure mode shipped the write through. + * + * Two halves, and the tests below pin both because either alone reintroduces a + * trap: + * + * 1. The record a predicate reads is TOTAL over the object's declared fields + * (`null` when neither the payload nor the prior record carries the key), on + * UPDATE as well as insert. Without this, every legitimate predicate 422s on + * any driver that stores only written columns — the shape the HotCRM report + * (hotcrm#630) came from. + * 2. What still cannot be evaluated over that total record — a typo'd key, a + * parse error — REJECTS the write, naming the rule and the offending key. + * Without this, a typo'd predicate is back to silently doing nothing. + */ +import { describe, it, expect, vi } from 'vitest'; +import { evaluateValidationRules } from './rule-validator'; +import { ValidationError } from './record-validator'; + +/** + * The HotCRM shape: a lead may only be closed as a duplicate while naming the + * surviving record. `survivor_lead_id` is DECLARED but the driver stores only + * written columns, so the prior record it hands back does not carry the key. + */ +const leadSchema = { + fields: { + status: { name: 'status', label: 'Status', type: 'text' }, + disqualification_reason: { name: 'disqualification_reason', label: 'Reason', type: 'text' }, + survivor_lead_id: { name: 'survivor_lead_id', label: 'Surviving lead', type: 'text' }, + }, + validations: [ + { + type: 'script' as const, + name: 'duplicate_disqualification_requires_survivor', + condition: { + dialect: 'cel', + source: + 'record.disqualification_reason == "duplicate" && record.survivor_lead_id == null', + }, + message: 'A duplicate must name the surviving lead.', + events: ['insert', 'update'] as Array<'insert' | 'update'>, + }, + ], +}; + +/** What a driver that stores only written columns returns: no `survivor_lead_id`. */ +const priorFromSparseDriver = { id: 'lead_1', status: 'open' }; + +describe('#4649 — the merged record is total on UPDATE', () => { + it('ENFORCES the rule when the prior record omits a declared key entirely', () => { + // Before #4649: `No such key: survivor_lead_id` → rule skipped → write + // allowed. The lead could be closed as a duplicate naming nobody. + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'update', + { previous: priorFromSparseDriver }, + ), + ).toThrow(/A duplicate must name the surviving lead/); + }); + + it('still allows the write when the payload DOES name the survivor', () => { + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate', survivor_lead_id: 'lead_9' }, + 'update', + { previous: priorFromSparseDriver }, + ), + ).not.toThrow(); + }); + + it('still allows the write when the survivor is already persisted', () => { + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'update', + { previous: { ...priorFromSparseDriver, survivor_lead_id: 'lead_9' } }, + ), + ).not.toThrow(); + }); + + it('behaves identically on insert (the two paths no longer diverge)', () => { + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'insert', + {}, + ), + ).toThrow(/A duplicate must name the surviving lead/); + }); + + it('materialises the `previous` binding too, so `previous.` is readable', () => { + const schema = { + fields: { + stage: { name: 'stage', label: 'Stage', type: 'text' }, + locked: { name: 'locked', label: 'Locked', type: 'boolean' }, + }, + validations: [ + { + type: 'script' as const, + name: 'no_reopen_once_locked', + // `previous.locked` is a declared column the sparse driver omitted. + condition: { dialect: 'cel', source: 'previous.locked == true && record.stage == "open"' }, + message: 'A locked record cannot be reopened.', + }, + ], + }; + // Prior carries neither key → before #4649 this faulted; it must now + // evaluate cleanly to "no violation", NOT reject. + expect(() => + evaluateValidationRules(schema, { stage: 'open' }, 'update', { previous: { id: 'r1' } }), + ).not.toThrow(); + // …and still reject when the prior really is locked. + expect(() => + evaluateValidationRules(schema, { stage: 'open' }, 'update', { previous: { id: 'r1', locked: true } }), + ).toThrow(/locked record cannot be reopened/); + }); + + it('does not mutate the caller\'s prior record (it is the engine\'s hookContext.previous)', () => { + const previous = { ...priorFromSparseDriver }; + try { + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'update', + { previous }, + ); + } catch { + /* expected */ + } + expect(previous).toEqual(priorFromSparseDriver); + expect('survivor_lead_id' in previous).toBe(false); + }); + + it('materialises DECLARED fields only — an undeclared key is not invented', () => { + const schema = { + fields: { a: { name: 'a', type: 'text' } }, + validations: [ + { + type: 'script' as const, + name: 'reads_undeclared', + condition: { dialect: 'cel', source: 'record.not_a_field == null' }, + message: 'unreachable', + }, + ], + }; + // If materialisation covered arbitrary keys this would evaluate TRUE and + // report the author's message; instead the typo is reported as a typo. + expect(() => evaluateValidationRules(schema, { a: '1' }, 'update', { previous: { a: '0' } })) + .toThrow(/could not be evaluated/); + }); +}); + +describe('#4649 — an unevaluable predicate fails CLOSED', () => { + const typoSchema = { + fields: { status: { name: 'status', label: 'Status', type: 'text' } }, + validations: [ + { + type: 'script' as const, + name: 'status_guard', + condition: { dialect: 'cel', source: 'record.stauts == "bad"' }, // typo + message: 'Status must not be bad.', + }, + ], + }; + + it('rejects the write and names the rule AND the missing key', () => { + let caught: ValidationError | null = null; + try { + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }); + } catch (err) { + caught = err as ValidationError; + } + expect(caught).toBeInstanceOf(ValidationError); + expect(caught!.message).toContain('status_guard'); + expect(caught!.message).toContain('stauts'); + const [field] = caught!.fields; + expect(field.code).toBe('rule_violation'); + expect(field.constraint).toMatchObject({ + rule: 'status_guard', + reason: 'unevaluable', + missingKey: 'stauts', + }); + }); + + it('does not present the author\'s message — the rule never said no', () => { + expect(() => + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }), + ).toThrow(/could not be evaluated/); + expect(() => + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }), + ).not.toThrow(/Status must not be bad/); + }); + + it('still logs the fault — the operator keeps the WARN, worded as a rejection', () => { + const warn = vi.fn(); + expect(() => + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { + previous: { status: 'ok' }, + logger: { warn }, + }), + ).toThrow(ValidationError); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]![0])).toMatch(/write rejected/); + expect(String(warn.mock.calls[0]![0])).not.toMatch(/— skipped/); + }); + + it('respects severity: a warning-severity broken rule logs and does NOT block', () => { + const warn = vi.fn(); + const advisory = { + ...typoSchema, + validations: [{ ...typoSchema.validations[0]!, severity: 'warning' as const }], + }; + expect(() => + evaluateValidationRules(advisory, { status: 'ok' }, 'update', { + previous: { status: 'ok' }, + logger: { warn }, + }), + ).not.toThrow(); + expect(warn).toHaveBeenCalled(); + }); + + it('rejects a parse-error predicate too, on insert as on update', () => { + const broken = { + fields: { a: { name: 'a', type: 'text' } }, + validations: [ + { type: 'script' as const, name: 'unparseable', condition: 'record.a &&', message: 'm' }, + ], + }; + expect(() => evaluateValidationRules(broken, { a: '1' }, 'insert', {})) + .toThrow(/rule 'unparseable' could not be evaluated/); + }); + + it('keeps the message single-line (the CEL source excerpt stays in the log)', () => { + let msg = ''; + try { + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }); + } catch (err) { + msg = (err as Error).message; + } + expect(msg).not.toContain('\n'); + }); +}); + +describe('#4649 — `has()` semantics over a materialised field, pinned', () => { + /** + * A materialised field is a PRESENT key holding `null`, so `has()` is TRUE — + * CEL's own rule, and what the insert path has always done. The app-side + * mitigation (hotcrm#630 wraps field reads in `has(...)`) must keep working, + * which it does as long as it pairs `has()` with a null test; a bare + * `!has(x)` is an emptiness test that never fires, and this pins that so the + * behaviour cannot drift under the apps relying on it. + */ + const schema = (source: string) => ({ + fields: { + status: { name: 'status', type: 'text' }, + survivor_lead_id: { name: 'survivor_lead_id', type: 'text' }, + }, + validations: [ + { type: 'script' as const, name: 'guarded', condition: { dialect: 'cel', source }, message: 'violated' }, + ], + }); + + const run = (source: string, data: Record, previous?: Record) => + () => evaluateValidationRules(schema(source), data, previous ? 'update' : 'insert', { previous }); + + it('a `has(x) && x == …` predicate evaluates (never faults) and is correct', () => { + // Guarded read over an absent declared key: evaluable, no violation. + expect(run('has(record.survivor_lead_id) && record.survivor_lead_id == "x"', { status: 'a' }, { id: 'r' })) + .not.toThrow(); + // …and fires when the value is really there. + expect(run('has(record.survivor_lead_id) && record.survivor_lead_id == "x"', { survivor_lead_id: 'x' }, { id: 'r' })) + .toThrow(ValidationError); + }); + + it('`has()` is TRUE for a materialised null — identically on insert and update', () => { + // The predicate IS `has(...)`: violated ⇔ has() is true. + expect(run('has(record.survivor_lead_id)', { status: 'a' }, { id: 'r' })).toThrow(ValidationError); + expect(run('has(record.survivor_lead_id)', { status: 'a' })).toThrow(ValidationError); + }); + + it('`has()` is FALSE — has() guards undeclared keys, not empty values', () => { + expect(run('has(record.no_such_column)', { status: 'a' }, { id: 'r' })).not.toThrow(); + }); + + /** + * The trap this pins is the one that cost two example objects in this very + * repo: `has(a) && has(b) && a < b` READS as a null guard and is not one. + * CEL cannot order-compare null, so the predicate aborts — and it aborted on + * every driver that returns its NULL columns, long before #4649. The + * rejection therefore has to teach the fix, not just report the fault. + */ + it('an ordering comparison guarded only by has() is rejected, and the message teaches `!= null`', () => { + let caught: ValidationError | null = null; + try { + evaluateValidationRules( + { + fields: { + start_date: { name: 'start_date', type: 'date' }, + end_date: { name: 'end_date', type: 'date' }, + }, + validations: [{ + type: 'cross_field' as const, + name: 'end_after_start', + fields: ['start_date', 'end_date'], + condition: { + dialect: 'cel', + source: 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date', + }, + message: 'End must be on or after start.', + }], + }, + { name: 'A project with no dates' }, + 'insert', + {}, + ); + } catch (err) { + caught = err as ValidationError; + } + expect(caught).toBeInstanceOf(ValidationError); + expect(caught!.message).toContain('end_after_start'); + expect(caught!.message).toContain("'!= null'"); + expect(caught!.message).toContain('has(x)'); + expect(caught!.fields[0]!.constraint).toMatchObject({ hint: 'null-comparison' }); + }); + + it('the same rule written with `!= null` evaluates cleanly on the same record', () => { + const schema = { + fields: { + start_date: { name: 'start_date', type: 'date' }, + end_date: { name: 'end_date', type: 'date' }, + }, + validations: [{ + type: 'cross_field' as const, + name: 'end_after_start', + fields: ['start_date', 'end_date'], + condition: { + dialect: 'cel', + source: 'record.start_date != null && record.end_date != null && record.end_date < record.start_date', + }, + message: 'End must be on or after start.', + }], + }; + expect(() => evaluateValidationRules(schema, { name: 'no dates' }, 'insert', {})).not.toThrow(); + expect(() => evaluateValidationRules(schema, { start_date: '2026-06-01', end_date: '2026-01-01' }, 'insert', {})) + .toThrow(/End must be on or after start/); + }); + + it('the emptiness test to write is `== null`, and it works on both paths', () => { + expect(run('record.survivor_lead_id == null', { status: 'a' }, { id: 'r' })).toThrow(ValidationError); + expect(run('record.survivor_lead_id == null', { status: 'a' })).toThrow(ValidationError); + expect(run('record.survivor_lead_id == null', { survivor_lead_id: 'x' }, { id: 'r' })).not.toThrow(); + }); +}); + +describe('#4649 — unchanged neighbours', () => { + it('a broken `regex` on a format rule stays fail-open (out of scope, deliberately)', () => { + const schema = { + fields: { code: { name: 'code', type: 'text' } }, + validations: [ + { type: 'format' as const, name: 'fmt', field: 'code', regex: '([', message: 'bad' }, + ], + }; + expect(() => evaluateValidationRules(schema, { code: 'x' }, 'update', { previous: { code: 'y' } })) + .not.toThrow(); + }); + + it('an uncompilable JSON Schema stays fail-open (out of scope, deliberately)', () => { + const schema = { + fields: { payload: { name: 'payload', type: 'json' } }, + validations: [ + { type: 'json_schema' as const, name: 'js', field: 'payload', schema: { type: 'not-a-type' }, message: 'bad' }, + ], + }; + expect(() => evaluateValidationRules(schema, { payload: { a: 1 } }, 'update', { previous: {} })) + .not.toThrow(); + }); + + it('a broken field-level `requiredWhen` stays fail-open (out of scope, deliberately)', () => { + const schema = { + fields: { + a: { name: 'a', type: 'text', requiredWhen: { dialect: 'cel', source: 'this is (( not valid' } }, + }, + validations: [], + }; + expect(() => evaluateValidationRules(schema, { a: null }, 'update', { previous: { a: null } })) + .not.toThrow(); + }); +}); diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 102454bcc3..c0244aade2 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -552,7 +552,10 @@ describe('script / cross_field predicates', () => { ).not.toThrow(); }); - it('fails open (no throw) on an un-evaluable predicate', () => { + // #4649 — this used to assert the opposite ("fails open"). A validation that + // could not be checked must not read as a pass; see rule-fail-closed.test.ts + // for the full contract. + it('fails CLOSED on an un-evaluable predicate (#4649)', () => { const schema = { validations: [ { @@ -565,7 +568,7 @@ describe('script / cross_field predicates', () => { }; expect(() => evaluateValidationRules(schema, { a: 1 }, 'update', { previous: { a: 0 } }), - ).not.toThrow(); + ).toThrow(/rule 'broken' could not be evaluated/); }); }); @@ -594,7 +597,7 @@ describe('introspection', () => { expect(needsPriorRecord(undefined)).toBe(false); }); - it('needsPriorRecord recurses into conditional branches', () => { + it('needsPriorRecord is true for any conditional, and recurses into its branches', () => { // conditional wrapping a cross_field → needs prior. const wrapsPrior = { validations: [ @@ -609,7 +612,11 @@ describe('introspection', () => { }; expect(needsPriorRecord(wrapsPrior)).toBe(true); - // conditional wrapping only a format → does not need prior. + // #4649 — a conditional wrapping only a format STILL needs the prior + // record: its `when` is evaluated against the merged record, so without the + // prior state it reads a PATCH as though it were the whole record. This + // assertion was `false` before #4649, which is why a `when` referencing an + // unchanged field used to fault and skip the guard entirely. const wrapsFormat = { validations: [ { @@ -621,7 +628,14 @@ describe('introspection', () => { }, ], }; - expect(needsPriorRecord(wrapsFormat)).toBe(false); + expect(needsPriorRecord(wrapsFormat)).toBe(true); + + // A rule set with no conditional and no prior-dependent rule still needs + // nothing — the fetch is not universal. + const formatOnly = { + validations: [{ type: 'format' as const, name: 'f', message: 'm', field: 'email', format: 'email' }], + }; + expect(needsPriorRecord(formatOnly)).toBe(false); }); }); @@ -821,9 +835,26 @@ describe('conditional enforcement', () => { ).not.toThrow(); }); - it('fails open on an un-evaluable when predicate', () => { + // #4649 — was "fails open". Neither branch ran, so the guard the author + // declared did not happen; that must not read as a pass. + it('fails CLOSED on an un-evaluable when predicate (#4649)', () => { const broken = { validations: [{ ...schema.validations[0], when: { dialect: 'cel', source: 'this is (( not valid' } }] }; - expect(() => evaluateValidationRules(broken, { account_type: 'enterprise', approver: null }, 'insert')).not.toThrow(); + expect(() => evaluateValidationRules(broken, { account_type: 'enterprise', approver: null }, 'insert')) + .toThrow(/could not be evaluated/); + }); + + // The outer conditional's severity still governs an unevaluable `when` — an + // advisory guard stays advisory even when it is broken. + it('an un-evaluable when on a warning-severity conditional does not block', () => { + const broken = { + validations: [{ + ...schema.validations[0], + severity: 'warning' as const, + when: { dialect: 'cel', source: 'this is (( not valid' }, + }], + }; + expect(() => evaluateValidationRules(broken, { account_type: 'enterprise', approver: null }, 'insert')) + .not.toThrow(); }); }); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 0d8c7dccd2..fc77b5e74e 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -54,13 +54,53 @@ * - `severity` → only `error` blocks the write. `warning` / `info` * are logged (best-effort) and never throw. * - * ## Fail-open for *broken* rules, fail-closed for *violated* rules + * ## Fail-CLOSED for unevaluable predicates (#4649) * - * A CEL predicate that cannot be evaluated (parse error, references an - * unbound variable, …) is a broken rule, not a violated one — it is logged - * and skipped rather than bricking every write to the object. A predicate - * that evaluates cleanly to "violated", or a transition that is definitively - * illegal, is fail-closed (the write is rejected). + * A validation exists to reject a write, so "the rule could not be checked" + * must never resolve to "the write goes through". Until #4649 a CEL predicate + * that faulted was logged at WARN and **skipped**: the rule stayed declared, + * listed in the metadata, and enforced nothing — on exactly the records whose + * shape triggered the fault. That inverts the guarantee the rule was written + * to give, and the only signal was a line in a log. + * + * Two changes close it, and they are load-bearing together: + * + * 1. **The record a predicate sees is TOTAL** — every field the object + * declares is present, `null` when absent from both the payload and the + * prior record ({@link materializeDeclaredFields}). This already held on + * insert (#1871); #4649 extends it to update and to the `previous` + * binding, so a predicate written against the object's DECLARED shape + * always has a value to read no matter what the driver returned. Without + * it, step 2 would 422 every legitimate predicate on any driver that + * stores only written columns. + * 2. **What still faults is rejected** — a predicate that cannot be evaluated + * over a total record references something the object does not declare (an + * author typo, an unbound variable, a parse error). That is a broken rule, + * and a broken *validation* fails closed: the write is rejected with a + * message naming the rule and the offending key. Without step 1 this alone + * would be unacceptable; without step 2 a typo'd predicate returns to + * silently doing nothing. + * + * `severity` still governs blocking: an unevaluable `warning`/`info` rule is + * logged, never thrown — advisory rules stay advisory. + * + * Deliberately NOT changed here: a broken `regex` (`format`), an uncompilable + * JSON Schema (`json_schema`), the FIELD-level predicates (`requiredWhen` / + * `readonlyWhen` / option `visibleWhen`), and the defensive `catch` around a + * rule that THROWS all keep their existing fail-open policy — #4649 scoped + * itself to the object-level validation predicates it was filed about, and a + * thrown exception is an engine fault the author has no remedy for (rejecting + * on it would brick every write with nothing to fix). Step 1 makes the + * field-level predicates evaluate far more often anyway, since their fault mode + * was the same missing key. + * + * One consequence worth knowing before writing a predicate: because a declared + * field is now always present, `has(record.)` is uniformly TRUE + * (a materialised `null` is a present key holding null — this is CEL's rule, + * and it is what the insert path has always done). `has()` therefore guards + * against an UNDECLARED key, not against an empty value; test emptiness with + * `record.x == null`. Pinned by test so the app-side `has(...)` idiom cannot be + * broken silently from under it. * * ## Prior-record plumbing * @@ -143,7 +183,11 @@ interface ConditionalRule extends BaseRule { */ interface RuleContext { data: Record; + /** Prior state overlaid with the PATCH, made TOTAL over the declared fields + * (#4649) — see {@link materializeDeclaredFields}. */ merged: Record; + /** The prior record, likewise made total (a COPY — the engine's own + * `hookContext.previous` must not gain materialised nulls). */ previous: Record | undefined; mode: Mode; logger: EvaluateRulesOptions['logger']; @@ -408,8 +452,10 @@ function isPreservableUnderAudit(name: string, def: ConditionalFieldDef): boolea /** * A rule needs the prior record if it reasons about the transition or compares * against unchanged fields (`state_machine` / `cross_field` / `script`), or if - * it is a `conditional` whose branches (or `when`) recursively do. `format` and - * `json_schema` only inspect the incoming value, so they never need it. + * it is a `conditional` — its `when` predicate is evaluated against the MERGED + * record, which is only a faithful view of the record with the prior state in + * hand. `format` and `json_schema` only inspect the incoming value, so they + * never need it. */ function ruleNeedsPrior(r: unknown): boolean { if (r == null || typeof r !== 'object') return false; @@ -419,13 +465,48 @@ function ruleNeedsPrior(r: unknown): boolean { } if (type === 'conditional') { const c = r as ConditionalRule; - // `when` is evaluated against the merged record; the branches may need prior - // state. Be conservative and fetch if either branch does. - return ruleNeedsPrior(c.then) || ruleNeedsPrior(c.otherwise); + // #4649 — a `conditional` needs the prior record as soon as it declares a + // `when`, not merely when a BRANCH does. `when` is evaluated against the + // merged record, so without the prior state it reads a PATCH as if it were + // the whole record: a `when` referencing an unchanged field used to fault + // (and skip the rule), and under fail-closed evaluation it would reject + // legitimate partial updates instead. Fetching is what makes the merged + // record total, and totality is what makes fail-closed safe. + return c.when != null || ruleNeedsPrior(c.then) || ruleNeedsPrior(c.otherwise); } return false; } +/** + * Materialise the object's DECLARED-but-absent fields as `null`, in place + * (#1871 for insert, #4649 for update and for the `previous` binding). + * + * CEL is strict about missing keys: `record.x` on a record that does not carry + * the key `x` aborts the whole predicate with `No such key`, which is NOT the + * same as reading `null`. Whether a key is carried is a property of the DRIVER, + * not of the data — a driver that stores only written columns returns a record + * missing every column the write never touched — so without this a predicate's + * evaluability depends on storage internals the author cannot see. + * + * Scope is deliberately the object's **declared fields only**. Materialising + * every key a predicate happens to name would defeat the fail-closed step: a + * typo'd `record.stauts` must stay unevaluable so it is reported, not silently + * read as `null` and quietly answered "no violation". + * + * `undefined` counts as absent (not just a missing key): CEL treats an own key + * holding `undefined` exactly as it treats no key at all. + */ +function materializeDeclaredFields( + record: Record, + fields: Record | undefined, +): Record { + if (!fields) return record; + for (const name of Object.keys(fields)) { + if (record[name] === undefined) record[name] = null; + } + return record; +} + /** Field-level conditional rules (B2): a field is required / read-only when its * CEL predicate is TRUE over the record. */ interface ConditionalFieldOption { @@ -580,20 +661,33 @@ export function evaluateValidationRules( const hasFieldRules = fieldsNeedPrior(fields); if (!hasRules && !hasFieldRules) return; - const previous = opts.previous ?? undefined; + const priorRecord = opts.previous ?? undefined; + // Is the record's persisted state actually in hand? On insert there is + // nothing to know (absence genuinely means "no value"); on update we know it + // only when the engine fetched the prior row. Without it, defaulting a field + // to `null` would not be materialising an absent value — it would be + // FABRICATING one that contradicts the stored row, so we leave the record as + // it is and let the (rare) unevaluable predicate fail closed. `ruleNeedsPrior` + // makes this path unreachable for every rule that reads the merged record. + const groundTruth = mode === 'insert' || priorRecord !== undefined; + // The `previous` CEL binding is made total too (#4649): a predicate reading + // `previous.x` for a declared column the driver did not return would + // otherwise fault — and now that faults are rejections, that would 422 a + // perfectly good rule. Copied, never mutated in place: the same object is the + // engine's `hookContext.previous`, which after-hooks observe. + const previous = mode === 'update' && priorRecord + ? materializeDeclaredFields({ ...priorRecord }, fields) + : priorRecord; // Merged view used by predicate rules: prior state overlaid with the PATCH, // so a rule referencing an unchanged field still sees its persisted value. const merged: Record = { ...(previous ?? {}), ...data }; - // #1871 — on INSERT, a field omitted entirely from the payload is absent from - // the record, so a `record.x == null` predicate sees a missing CEL key (which - // does not equal null) and silently can't match. Default declared-but-absent - // fields to null so an omitted optional reads as null — matching an explicit - // `null` and the UPDATE path (where the prior record already supplies them). - if (mode === 'insert' && fields) { - for (const name of Object.keys(fields)) { - if (!(name in merged)) merged[name] = null; - } - } + // #1871 (insert) / #4649 (update) — a field the payload omits is absent from + // the CEL scope, so `record.x == null` sees a missing key (which does not + // equal null) and aborts the whole predicate. Default declared-but-absent + // fields to null so an omitted optional reads as null, identically on insert + // and update: what a predicate can read is the object's DECLARED shape, not + // whatever subset of columns this driver happened to return. + if (groundTruth) materializeDeclaredFields(merged, fields); const ctx: RuleContext = { data, merged, previous, mode, logger: opts.logger, fields, messages: opts.messages }; const errors: FieldValidationError[] = []; @@ -777,10 +871,88 @@ function checkStateMachine( return null; } +/** `No such key: ` is cel-js's word for "the predicate read something the + * record does not carry" — the single most useful fact to put in front of the + * author, since after materialisation it can only mean an UNDECLARED key. */ +const NO_SUCH_KEY_RE = /No such key:\s*([A-Za-z_$][\w$]*)/; + +/** + * The OTHER way a predicate written against a total record still faults: an + * ordering comparison (`<`, `>`, `<=`, `>=`) or arithmetic over a value that is + * `null`. CEL has no overload for it, so the whole predicate aborts. + * + * This one deserves its own sentence because the obvious guard does not work: + * `has(x)` is TRUE for a declared field holding `null` (CEL asks whether the key + * is PRESENT, not whether it has a usable value), so `has(a) && has(b) && a < b` + * still faults the moment either is null — on any driver that returns its NULL + * columns, which is most of them. Such a rule never enforced anything on those + * rows; #4649 is what makes that visible instead of silent. + */ +const NULL_OVERLOAD_RE = /no such overload/i; + +/** + * One-line summary of a CEL fault. The engine appends a source excerpt and a + * caret line to `message`, which is right for a log and wrong for an API error, + * so only the first line travels. + */ +function faultSummary(error: { kind: string; message: string }): string { + const first = String(error.message ?? '').split('\n')[0]!.trim(); + return `${error.kind}: ${first || 'unknown error'}`; +} + +/** + * The rejection a predicate that CANNOT BE EVALUATED produces (#4649). + * + * Not a violation — the rule never got to say yes or no — but it rejects the + * write all the same, because a validation whose verdict is unknown may not be + * read as "allowed". The message names the rule and, when the fault is a + * missing key, that key plus the two ways to fix it; `constraint` carries the + * same facts machine-readably so a client need not parse prose. + * + * The wire `code` stays `rule_violation`: the field-error catalog (ADR-0114, + * `packages/spec`) is closed and a broken rule is still "a declared rule + * rejected this write" from every consumer's point of view. `constraint.reason` + * is what distinguishes the two for anyone who cares. + */ +function unevaluableRuleError( + ruleName: string, + field: string, + error: { kind: string; message: string }, + what: 'predicate' | 'when-predicate', +): FieldValidationError { + const raw = String(error.message ?? ''); + const summary = faultSummary(error); + const missingKey = NO_SUCH_KEY_RE.exec(raw)?.[1]; + const nullOverload = !missingKey && NULL_OVERLOAD_RE.test(raw) && /null/.test(raw); + let detail = ''; + if (missingKey) { + detail = ` The ${what} reads '${missingKey}', which this object does not declare — fix the rule's condition, or declare the field.`; + } else if (nullOverload) { + detail = + ` The ${what} compares a value that is null. Guard it with '!= null'` + + ` — 'has(x)' does NOT do that: a declared field holding null is still PRESENT, so has(x) is true.`; + } + return { + field, + code: 'rule_violation', + message: + `Validation rule '${ruleName}' could not be evaluated (${summary}) — write rejected.${detail}`, + constraint: { + rule: ruleName, + reason: 'unevaluable', + fault: summary, + ...(missingKey ? { missingKey } : {}), + ...(nullOverload ? { hint: 'null-comparison' } : {}), + }, + }; +} + /** * CEL predicate check (`script` / `cross_field`). The predicate expresses the - * *failure* condition: if it evaluates TRUE the rule is violated. An - * un-evaluable predicate is treated as a broken rule (logged, skipped). + * *failure* condition: if it evaluates TRUE the rule is violated. A predicate + * that cannot be evaluated — over a record already made total for every + * declared field — is a broken rule, and a broken validation is **fail-closed** + * (#4649): it rejects the write rather than waving it through. */ function checkPredicate( rule: PredicateRule, @@ -794,16 +966,20 @@ function checkPredicate( previous: previous ?? undefined, }); + const field = rule.fields?.[0] ?? '_record'; + if (!result.ok) { + // Still logged — the operator needs the fault in the log even though the + // caller now gets it in the response. Note the verb: rejected, not skipped. logger?.warn?.( - `Validation rule '${rule.name}' predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — skipped`, + `Validation rule '${rule.name}' predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — write rejected (#4649)`, ); - return null; + return unevaluableRuleError(rule.name, field, result.error, 'predicate'); } if (result.value === true) { return { - field: rule.fields?.[0] ?? '_record', + field, code: 'rule_violation', message: rule.message, }; @@ -933,9 +1109,12 @@ function checkJsonSchema( /** * Conditional check (`conditional`). Evaluates the `when` predicate against the * merged record, then recurses into `then` (true) or `otherwise` (false) via - * `evaluateRule`. An un-evaluable `when` is a broken rule (logged, fail-open). - * The nested rule supplies the violation (field/code/message); the *outer* - * conditional's `severity` governs whether it blocks (handled by the caller). + * `evaluateRule`. An un-evaluable `when` is **fail-closed** (#4649) for the same + * reason a `script` predicate is: neither branch ran, so the guard the author + * declared did not happen, and a validation that did not happen must not read as + * a pass. The nested rule supplies the violation (field/code/message) when the + * `when` DOES evaluate; the *outer* conditional's `severity` governs whether + * either outcome blocks (handled by the caller). */ function checkConditional( rule: ConditionalRule, @@ -948,9 +1127,9 @@ function checkConditional( if (!result.ok) { ctx.logger?.warn?.( - `Validation rule '${rule.name}' when-predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — skipped`, + `Validation rule '${rule.name}' when-predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — write rejected (#4649)`, ); - return null; + return unevaluableRuleError(rule.name, '_record', result.error, 'when-predicate'); } const branch = result.value === true ? rule.then : rule.otherwise;