diff --git a/.changeset/24512-conditional-when-hydration.md b/.changeset/24512-conditional-when-hydration.md new file mode 100644 index 0000000000..4fc11a5932 --- /dev/null +++ b/.changeset/24512-conditional-when-hydration.md @@ -0,0 +1,31 @@ +--- +'@objectstack/objectql': minor +'@objectstack/lint': minor +--- + +fix(objectql,lint)!: a `conditional` validation rule's `when` reads one hop through a reference, the way a rule's `condition` does + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing, at the save door and at write time), shipped as `minor` under the launch-window convention for breaking changes. + +A `conditional` rule's `when` that reads through a lookup (`record.account.status == 'active'`) now works. The engine loads the related row for it, as it does for a `script` or `cross_field` rule's `condition`, and `when` picks `then` or `otherwise` on that row. Before, `when` met the stored id, so the read faulted and every write the rule guarded was refused with "The when-predicate reads 'status', which this object does not declare". A `when` nested in a `then` or `otherwise` is served the same way. A sibling rule keeps its own verdict when a `when` cannot be evaluated. + +`@objectstack/objectql`, at write time: + +- FROM: `when: record.account.status == 'active'` refused every write it guarded as unevaluable. TO: it is judged on the related row. +- FROM: a null-safe spelling (`record.account.?status.orValue('') == 'active'`, `has(record.account.status) && …`) read the stored id and silently took `otherwise` for every account. TO: it is judged on the related row. +- FROM: on a write whose reference is empty, a null-safe `when`, or one that tests `record.account != null` in the same expression, evaluated to false and judged `otherwise`. TO: the write is refused (`400 VALIDATION_FAILED`, `_record` `rule_violation`), as a `condition` reading through an empty reference is. The refusal names the guard. +- A `when` that reads a second hop, a field the related object does not declare, or the reference both through it and as a value is refused with a sentence that names the related object and field, instead of the generic one. A `when` that faults on a delete's reference cleanup names the delete, as a `condition` does. +- The guard spelling `when: record.account != null` reads the stored id, as before. + +`@objectstack/lint`, where a rule is written: `os validate`, `os build` and the object save door (`PUT /api/v1/meta/object/:name`) run the same pass, under the existing `EXPRESSION_INVALID` rule. They now judge a `when`, top level and nested, with the checks a `condition` gets. Each of these saved before (`200`) and is refused now (`422 INVALID_METADATA` at the save door), located at the `when` slot. At run time each refused every write it guarded, and the third also took `otherwise` on an empty reference: + +- FROM: a second hop (`record.account.owner.email != null`), saved. TO: refused. +- FROM: a field the target does not declare (`record.account.region == 'north'`), saved. TO: refused, "unknown field `region` on ``". +- FROM: the reference read both through it and as a value (`record.account != null && record.account.status == 'active'`), saved. TO: refused, naming the guard. +- FROM: a hop into a stored-metadata-body table, saved. TO: refused. + +What to do. To skip a rule while the reference is empty, keep the reference test in its own `when` and nest the rule that reads through it: a `conditional` whose `when` is `record.account != null`, with your `conditional` as its `then`. To refuse an empty reference, make the field `required: true`. Read a field the target declares, and read one hop. diff --git a/packages/lint/src/runtime-gate.object-validation-writes.test.ts b/packages/lint/src/runtime-gate.object-validation-writes.test.ts index f6c67ca4da..2d25a3c69e 100644 --- a/packages/lint/src/runtime-gate.object-validation-writes.test.ts +++ b/packages/lint/src/runtime-gate.object-validation-writes.test.ts @@ -39,8 +39,8 @@ * down, in `os build` and at this door alike. The pass now runs the same * `check()` on every nested predicate, at the location the null-guard gate * already gives it (`validation rule 'outer' then → 'inner'`), with the - * relationship-traversal checks on a nested `condition` (ObjectQL hydrates it) - * and not on a nested `when` (it does not). The second describe block below + * relationship-traversal checks on a nested `condition` and — since #24512 — + * on a nested `when` (ObjectQL hydrates both). The second describe block below * pins it; its protocol half is the #22042 block of the same protocol file. */ import { describe, expect, it } from 'vitest'; @@ -236,7 +236,7 @@ const fxRef = (validations: unknown[]) => { const base = fxRule(validations); return { ...base, fields: { ...base.fields, account: { type: 'lookup', label: 'Account', reference: 'fx_rule' } } }; }; -/** Reads more than one relationship hop — a shape `checkPredicate` refuses, and `checkConditional` never judges. */ +/** Reads more than one relationship hop — a shape `checkPredicate` and (#24512) `checkConditional` refuse. */ const MULTI_HOP = 'record.account.owner.email != null'; const HYDRATION = { type: 'conditional', @@ -311,16 +311,21 @@ describe('#22042 — a `conditional` rule\'s nested predicates meet the same ver ]); }); - it('the traversal checks follow the evaluator per slot: ON for a nested `condition`, OFF for a nested `when`', () => { + it('the traversal checks follow the evaluator per slot: ON for a nested `condition` and, since #24512, a nested `when`', () => { const atBuild = buildFindings(fxRef([HYDRATION])); - // `checkPredicate` refuses a read deeper than one hop at any depth, so the build says so… - expect(atBuild.map((f) => f.where), dump(atBuild)).toEqual(["object 'fx_rule' · validation rule 'outer' then → 'inner'"]); - expect(atBuild[0]!.message).toContain('ONE hop'); - // …and `checkConditional` never hydrates a `when`, so the same source there earns no - // traversal prescription — exactly as the top-level `when` site is opted out. + // `checkPredicate` refuses a read deeper than one hop at any depth, and + // `checkConditional` evaluates a `when` on the same hydrated scope, so the + // build says so for both slots… + expect(atBuild.map((f) => f.where), dump(atBuild)).toEqual([ + "object 'fx_rule' · validation rule 'outer' then → 'inner'", + "object 'fx_rule' · validation rule 'outer' otherwise → 'mid' when-predicate", + ]); + for (const f of atBuild) expect(f.message).toContain('ONE hop'); + // …and at the top-level `when` site too. const topWhen = buildFindings(fxRef([{ ...HYDRATION, when: MULTI_HOP, then: NESTED_VALID.otherwise, otherwise: undefined }])); - expect(topWhen, dump(topWhen)).toEqual([]); + expect(topWhen.map((f) => f.where), dump(topWhen)).toEqual(["object 'fx_rule' · validation 'outer' when"]); + expect(topWhen[0]!.message).toContain('ONE hop'); expect(expressionFindings(gateObject(fxRef([HYDRATION])).errors)).toEqual(atBuild); }); @@ -382,3 +387,68 @@ describe('#24468 — the object door refuses a rule hop to a field the target do expect(expressionFindings(result.advisories), dump(result)).toEqual([]); }); }); + +describe('#24512 — the object door judges a `conditional` rule\'s `when` hop as it judges a `condition`\'s', () => { + /** The lookup's target, delivered by ANOTHER package the written one depends on. */ + const account = { + name: 'fx_account', label: 'Account', sharingModel: 'private', _packageId: 'com.example.accounts', + fields: { status: { type: 'text', label: 'Status' } }, + }; + const order = (validations: unknown[]) => ({ + name: 'fx_order', + label: 'Order', + sharingModel: 'private', + fields: { + amount: { type: 'number', label: 'Amount' }, + account: { type: 'lookup', label: 'Account', reference: 'fx_account' }, + }, + validations, + }); + const gate = (when: string, name = 'gate') => ({ + type: 'conditional', name, when, message: 'x', + then: { type: 'script', name: 'cap', condition: 'record.amount != null && record.amount > 100', message: 'y' }, + }); + const gateOrder = (validations: unknown[]) => runRuntimeAuthoringRules({ + type: 'object', + item: order(validations), + context: { objects: [account] }, + packageScope: { packageId: 'com.example.orders', dependencies: ['com.example.accounts'] }, + }); + + it('the fixtures are spec-valid: each refusal below is the expression verdict, not the schema\'s', () => { + for (const when of ["record.account.region == 'north'", 'record.account.owner.email != null', "record.account.status == 'open'"]) { + const parsed = ObjectSchema.safeParse(order([gate(when), { type: 'conditional', name: 'outer', when: 'record.amount != null', message: 'x', then: gate(when, 'inner') }])); + expect(parsed.success, dump(parsed.error?.issues)).toBe(true); + } + }); + + it('⭐ LIT — a `when` reading a field the target does not declare is REFUSED under EXPRESSION_INVALID, naming the slot, the field and the target', () => { + const result = gateOrder([gate("record.account.region == 'north'")]); + + const errs = expressionFindings(result.errors); + expect(errs, dump(result)).toHaveLength(1); + const where = "object 'fx_order' · validation 'gate' when"; + expect(errs[0]).toMatchObject({ rule: EXPRESSION_INVALID, severity: 'error', where, path: where }); + expect(errs[0]!.message).toContain('unknown field `region` on `fx_account`'); + expect(errs[0]!.message).toContain('the rule has no verdict'); + }); + + it('⭐ LIT — a `when` reading a second hop is REFUSED, and a nested `when` is judged too', () => { + const result = gateOrder([{ + type: 'conditional', name: 'outer', when: 'record.amount != null', message: 'x', + then: gate('record.account.owner.email != null', 'inner'), + }]); + + const errs = expressionFindings(result.errors); + expect(errs.map((e) => e.where), dump(result)).toEqual(["object 'fx_order' · validation rule 'outer' then → 'inner' when-predicate"]); + expect(errs[0]).toMatchObject({ rule: EXPRESSION_INVALID, severity: 'error' }); + expect(errs[0]!.message).toContain('ONE hop'); + }); + + it('⭐ CONTROL — a `when` reading a field the target declares publishes clean', () => { + const result = gateOrder([gate("record.account.status == 'open'")]); + + expect(expressionFindings(result.errors), dump(result)).toEqual([]); + expect(expressionFindings(result.advisories), dump(result)).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-expressions.rule-one-hop.test.ts b/packages/lint/src/validate-expressions.rule-one-hop.test.ts index 3d7ae89378..0196f95439 100644 --- a/packages/lint/src/validate-expressions.rule-one-hop.test.ts +++ b/packages/lint/src/validate-expressions.rule-one-hop.test.ts @@ -71,3 +71,58 @@ describe('[#24468] a validation rule reads one hop through a reference', () => { expect(errorsOf("record.account.region == 'north'", 'hop_elsewhere')).toEqual([]); }); }); + +/** + * [#24512] A `conditional` rule's `when` is hydrated one hop by ObjectQL's + * `checkConditional`, as a `condition` is by `checkPredicate`, so the same hop + * checks judge it — at the top level and in a nested `conditional` — under the + * same `expression-invalid` rule. + */ +describe('[#24512] a `conditional` rule\'s `when` reads one hop through a reference', () => { + const WHEN_WHERE = "object 'hop_order' · validation 'gate' when"; + const NESTED_WHERE = "object 'hop_order' · validation rule 'outer' then → 'gate' when-predicate"; + const gate = (when: string) => ({ + name: 'gate', type: 'conditional', severity: 'error', message: 'x', when, + then: { name: 'cap', type: 'script', severity: 'error', message: 'y', condition: 'record.amount != null && record.amount > 100' }, + }); + const withRule = (rule: Record) => ({ ...order('true'), validations: [rule] }); + const errorsAt = (rule: Record) => + validateStackExpressions(specValid({ objects: [account, withRule(rule)] })) + .filter((i) => (i.severity ?? 'error') === 'error'); + const outer = (when: string) => ({ + name: 'outer', type: 'conditional', severity: 'error', message: 'x', when: 'record.amount != null', then: gate(when), + }); + + it('admits a `when` reading a field the target declares, top level and nested', () => { + expect(errorsAt(gate("record.account.status == 'active'"))).toEqual([]); + expect(errorsAt(outer("record.account.status == 'active'"))).toEqual([]); + }); + + it('refuses a `when` reading a field the target does not declare, naming the slot, the field and the target', () => { + for (const [rule, where] of [[gate("record.account.region == 'north'"), WHEN_WHERE], [outer("record.account.region == 'north'"), NESTED_WHERE]] as const) { + const errors = errorsAt(rule); + expect(errors.map((e) => e.where)).toEqual([where]); + expect(errors[0].message).toMatch(/^unknown field `region` on `hop_account`/); + expect(errors[0].message).toContain('through the reference `record.account`'); + } + }); + + it('refuses a `when` reading a second hop, naming the slot', () => { + for (const [rule, where] of [[gate("record.account.owner.email == 'x'"), WHEN_WHERE], [outer("record.account.owner.email == 'x'"), NESTED_WHERE]] as const) { + const errors = errorsAt(rule); + expect(errors.map((e) => e.where)).toEqual([where]); + expect(errors[0].message).toContain('read through more than one relationship hop'); + } + }); + + it('refuses a `when` reading the reference both through it and as a value, naming the guard', () => { + const errors = errorsAt(gate("record.account != null && record.account.status == 'active'")); + expect(errors.map((e) => e.where)).toEqual([WHEN_WHERE]); + expect(errors[0].message).toContain('is read BOTH through the relationship'); + expect(errors[0].message).toContain('`conditional` rule whose `when` is `record.account != null`'); + }); + + it('CONTROL — the guard spelling reads the stored id and is admitted', () => { + expect(errorsAt(gate('record.account != null'))).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 3c4940e745..a8c1b42278 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -544,8 +544,6 @@ function celSourceOf(raw: unknown): string | undefined { interface RulePredicate { label: string; raw: unknown; - /** `condition` (a `script` / `cross_field` rule's) or `when` (a `conditional` rule's). */ - slot: 'condition' | 'when'; /** 0 for the rule itself; 1 inside its `then` / `otherwise`, and so on down. */ depth: number; } @@ -568,9 +566,9 @@ function rulePredicates(rule: AnyRec, path: string, depth = 0): RulePredicate[] // author who wrote both `condition` and a rejected alias had their canonical // predicate short-circuited away and the alias validated instead (#5017). const main = rule.condition; - if (main != null) out.push({ label: `validation rule ${here}`, raw: main, slot: 'condition', depth }); + if (main != null) out.push({ label: `validation rule ${here}`, raw: main, depth }); if (rule.when != null) { - out.push({ label: `validation rule ${here} when-predicate`, raw: rule.when, slot: 'when', depth }); + out.push({ label: `validation rule ${here} when-predicate`, raw: rule.when, depth }); } for (const branch of ['then', 'otherwise'] as const) { const nested = rule[branch]; @@ -1946,7 +1944,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression /** * [#24316] Refuse a one-hop read through a reference into a * stored-metadata-body table, at the two sites that serve a hop: a - * validation rule's hydrated `condition` ({@link check} with + * validation rule's hydrated `condition` or `when` ({@link check} with * `traversalHydration`) and a formula field's `expression` * ({@link judgeFieldFormula}). ObjectQL's one-hop loader never reads that * hop, so as written the field reads null or the rule refuses every write; @@ -1984,8 +1982,8 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression * * [#24468] The two sites that serve a hop share it: a formula field's * `expression` ({@link judgeFieldFormula}) and a validation rule's hydrated - * `condition` ({@link check} with `traversalHydration`), where the rule has - * no verdict and refuses every write it judges. + * `condition` or `when` ({@link check} with `traversalHydration`), where the + * rule has no verdict and refuses every write it judges. * * Judged against the target's field index — its declared fields and the * system columns the platform injects on it, the set the registry carries — @@ -2163,7 +2161,8 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression /** * [#18682] Opt this ONE site in to the relationship-traversal conflict * checks. True only where the traversal is actually SERVED — object - * validation rules, which ObjectQL's `checkPredicate` hydrates (a formula + * validation rules' `condition` and `when`, which ObjectQL's + * `checkPredicate` and `checkConditional` hydrate (#24512) (a formula * `expression`, the other served site, is judged by `judgeFieldFormula`, * which does not go through this helper). Everywhere * else the checks' prescription ("write `record..id`") is false, @@ -2787,15 +2786,13 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // bare refs are flagged (#1928). // [#18682] The sites where a relationship traversal is SERVED: these are // the `script` / `cross_field` conditions ObjectQL's `checkPredicate` - // hydrates. `traversalHydration` is passed here, on a nested - // `condition` below (#22042), and on a formula field's `expression` - // (`judgeFieldFormula`, #24197), and NOWHERE else. + // hydrates, and (#24512) a `conditional` rule's `when`, which + // `checkConditional` evaluates on the same hydrated scope. + // `traversalHydration` is passed on those two slots here and nested below + // (#22042), and on a formula field's `expression` (`judgeFieldFormula`, + // #24197), and NOWHERE else. check(where, rule.condition, objectName, 'record', undefined, true); - // `conditional` rules carry a nested `when` predicate (record-scoped). - // ⚠️ `when` is evaluated by `checkConditional` WITHOUT hydration today, so - // it is opted OUT: a traversal there faults, and the conflict checks' - // prescription would not repair it. - check(`${where} when`, (rule as AnyRec).when, objectName, 'record'); + check(`${where} when`, (rule as AnyRec).when, objectName, 'record', undefined, true); const predicates = rulePredicates(rule, ''); // [#22042] The same verdict one level down, and every level below it: a // `conditional` rule's `then` / `otherwise` is a rule the evaluator runs @@ -2804,15 +2801,13 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // at the label `rulePredicates` builds, the location the null-guard gate // below already gives the same predicate. Depth 0 is skipped: the rule's // own `condition` / `when` met `check()` above, under their own location. - // Hydration follows the evaluator per slot, as at the top level: a - // nested `condition` is a `script` / `cross_field` rule's, which - // ObjectQL's `collectPredicateRelationships` reaches inside a - // `conditional` and `checkPredicate` hydrates; a nested `when` is a - // nested `conditional`'s, which `checkConditional` evaluates WITHOUT - // hydration, so it is opted out like the top-level `when`. + // Hydration follows the evaluator, as at the top level: ObjectQL's + // `collectPredicateRelationships` reaches a nested `condition` and a + // nested `when` alike, and both are evaluated hydrated, so both slots + // meet the traversal checks. for (const p of predicates) { if (p.depth === 0) continue; - check(`object '${objectName}' · ${p.label}`, p.raw, objectName, 'record', undefined, p.slot === 'condition'); + check(`object '${objectName}' · ${p.label}`, p.raw, objectName, 'record', undefined, true); } // #4763 — null-guard gate over every predicate the rule carries, nested // `then`/`otherwise` branches included. diff --git a/packages/objectql/src/engine-predicate-relationship.test.ts b/packages/objectql/src/engine-predicate-relationship.test.ts index fa3637d4a8..bdbb584302 100644 --- a/packages/objectql/src/engine-predicate-relationship.test.ts +++ b/packages/objectql/src/engine-predicate-relationship.test.ts @@ -284,6 +284,80 @@ describe('#18682 — engine-produced relationship bindings', () => { }); }); + // [#24512] A `conditional` rule's `when` reading one hop through the lookup: + // the engine preloads the hop for it and `checkConditional` evaluates `when` + // on the related row, so the rule picks its branch instead of refusing every + // write it guards as unevaluable. + describe('[#24512] a `conditional` rule\'s `when` reads through the lookup', () => { + const THEN = 'GATE-THEN: partner deals are capped at 10000.'; + const OTHERWISE = 'GATE-OTHERWISE: direct deals start at 100.'; + const gate = (when: string, name = 'partner_gate') => ({ + name, type: 'conditional', severity: 'error', message: 'unused', when, + then: { name: 'gate_cap', type: 'script', severity: 'error', message: THEN, condition: 'record.amount > 10000' }, + otherwise: { name: 'gate_floor', type: 'script', severity: 'error', message: OTHERWISE, condition: 'record.amount < 100' }, + }); + const register = (validations: unknown[]) => { + engine.registry.registerObject({ + name: 'crm_opportunity', + fields: { + name: { type: 'text' }, amount: { type: 'number' }, + account: { type: 'lookup', reference: 'crm_account' }, + }, + validations, + } as any, 'test-package'); + d.calls.length = 0; + }; + /** The rejection envelope, or `null` when the write is accepted. */ + const insert = async (data: Record): Promise<{ code?: string; fields: any[] } | null> => { + try { + await engine.insert('crm_opportunity', data, { context: ACTING } as any); + return null; + } catch (e) { + const err = e as { code?: string; fields?: any[] }; + return { code: err.code, fields: err.fields ?? [] }; + } + }; + const messagesOf = (r: { fields: any[] } | null): string[] => (r?.fields ?? []).map((f) => String(f.message)); + + it('picks `then` for a partner account and `otherwise` for any other, on the real related row', async () => { + register([gate("record.account.type == 'partner'")]); + const partner = await insert({ name: 'A', amount: 50000, account: 'acc_p' }); + expect(partner?.code).toBe('VALIDATION_FAILED'); + expect(partner?.fields).toEqual([expect.objectContaining({ field: '_record', code: 'rule_violation', message: THEN })]); + const direct = await insert({ name: 'B', amount: 50, account: 'acc_d' }); + expect(direct?.code).toBe('VALIDATION_FAILED'); + expect(direct?.fields).toEqual([expect.objectContaining({ field: '_record', code: 'rule_violation', message: OTHERWISE })]); + // Each branch admits what it does not refuse, and the other branch is not run. + expect(await insert({ name: 'C', amount: 50, account: 'acc_p' })).toBeNull(); + expect(await insert({ name: 'D', amount: 50000, account: 'acc_d' })).toBeNull(); + }); + + it('preloads the `when` hop: one system read, projecting only the column `when` names', async () => { + register([gate("record.account.type == 'partner'")]); + await insert({ name: 'A', amount: 10, account: 'acc_d' }); + expect(relatedReads()).toHaveLength(1); + expect([...(relatedReads()[0].ast?.fields ?? [])].sort()).toEqual(['id', 'type']); + }); + + it('a `when` naming an undeclared related field refuses alone; a sibling `when` through the same lookup keeps its verdict', async () => { + register([gate("record.account.type == 'partner'"), gate("record.account.not_a_column == 'x'", 'sneaky_gate')]); + const direct = await insert({ name: 'A', amount: 50000, account: 'acc_d' }); + expect(direct?.code).toBe('VALIDATION_FAILED'); + expect(direct?.fields).toEqual([expect.objectContaining({ + field: '_record', + code: 'rule_violation', + constraint: expect.objectContaining({ rule: 'sneaky_gate', reason: 'unevaluable' }), + })]); + expect(messagesOf(direct)[0]).toContain("'crm_account' declares no 'not_a_column'"); + // The valid gate, on a partner account, refuses with its own branch's message. + const partner = await insert({ name: 'B', amount: 50000, account: 'acc_p' }); + expect(messagesOf(partner)).toContain(THEN); + expect(partner?.fields.map((f) => f.constraint?.rule ?? null)).toEqual([null, 'sneaky_gate']); + // The shared read never names the undeclared column. + for (const call of relatedReads()) expect(call.ast?.fields ?? []).not.toContain('not_a_column'); + }); + }); + // The refusal is about the WRITE, never about the value. A caller can infer a // value by observing refusals — an accepted, deliberately narrow channel — so // nothing may make the refusal more informative than "this rule refused". diff --git a/packages/objectql/src/validation/rule-relationship-traversal.test.ts b/packages/objectql/src/validation/rule-relationship-traversal.test.ts index f301e1a501..ce78d6c5ff 100644 --- a/packages/objectql/src/validation/rule-relationship-traversal.test.ts +++ b/packages/objectql/src/validation/rule-relationship-traversal.test.ts @@ -485,3 +485,177 @@ describe('[#24468] undeclared related fields are judged per RULE, never per refe { related: { account: unavailable('undeclared-field', ['region']) } as never })).toThrow(/declares no 'region'/); }); }); + +// [#24512] A `conditional` rule's `when` reads one hop through a reference the +// way a `script` condition does: the collector names the hop (so the engine +// preloads it), and `checkConditional` evaluates `when` on the scope +// `resolveTraversalScope` decides — the related row, or a refusal naming why +// there is none. Before, `when` met the bare id and every write it guarded was +// refused as unevaluable. +describe('[#24512] a `conditional` rule\'s `when` reads one hop through a reference', () => { + const THEN = 'GATE-THEN: partner deals are capped at 10000.'; + const OTHERWISE = 'GATE-OTHERWISE: direct deals start at 100.'; + const SIBLING = 'SIBLING: deals over 100000 need approval.'; + /** The card's shape: a `conditional` whose `when` reads the account's type. */ + const gate = (when: string, name = 'partner_gate') => ({ + name, type: 'conditional', severity: 'error', message: 'unused', when, + then: { name: 'gate_cap', type: 'script', severity: 'error', message: THEN, condition: 'record.amount > 10000' }, + otherwise: { name: 'gate_floor', type: 'script', severity: 'error', message: OTHERWISE, condition: 'record.amount < 100' }, + }); + const sibling = { name: 'big_deal', type: 'script', severity: 'error', message: SIBLING, condition: 'record.amount > 100000' }; + const schemaOf = (...validations: unknown[]) => ({ fields: opportunity.fields, validations }); + const PARTNER = { account: row({ id: 'acc_1', type: 'partner' }) }; + const DIRECT = { account: row({ id: 'acc_1', type: 'direct' }) }; + + /** The thrown envelope, or `null` when the write is accepted. */ + const verdict = ( + schema: unknown, data: Record, related?: Record, + ): ValidationError | null => { + try { + evaluateValidationRules(schema as any, data, 'insert', { related: related as never }); + return null; + } catch (e) { + if (e instanceof ValidationError) return e; + throw e; + } + }; + const messagesOf = (e: ValidationError | null): string[] => (e?.fields ?? []).map((f) => f.message); + const deal = (amount: number) => ({ name: 'A', amount, account: 'acc_1' }); + + describe('the collector reaches `when`, so the engine preloads its hop', () => { + it('names the hop a top-level `when` takes', () => { + const map = collectPredicateRelationships(schemaOf(gate("record.account.type == 'partner'")) as any); + expect([...map.keys()]).toEqual(['account']); + expect([...map.get('account')!]).toEqual(['type']); + }); + + it('names the hop a `when` nested in a branch takes', () => { + const map = collectPredicateRelationships(schemaOf({ + name: 'outer', type: 'conditional', when: 'record.amount > 0', + otherwise: gate("record.account.tier == 'gold'", 'inner'), + }) as any); + expect([...map.keys()]).toEqual(['account']); + expect([...map.get('account')!]).toEqual(['tier']); + }); + + it('names nothing for the guard spelling, which reads the stored id and no hop', () => { + expect(collectPredicateRelationships(schemaOf(gate('record.account != null')) as any).size).toBe(0); + }); + }); + + describe('`when` is evaluated on the related row and picks its branch', () => { + const schema = schemaOf(gate("record.account.type == 'partner'")); + + it('a partner account runs `then`', () => { + const refused = verdict(schema, deal(50000), PARTNER); + expect(refused?.code).toBe('VALIDATION_FAILED'); + expect(refused?.fields).toEqual([expect.objectContaining({ field: '_record', code: 'rule_violation', message: THEN })]); + expect(refused?.fields[0]?.constraint?.reason).toBeUndefined(); + // `then` admits what it does not refuse, and `otherwise` is not run. + expect(verdict(schema, deal(50), PARTNER)).toBeNull(); + }); + + it('any other account runs `otherwise`', () => { + const refused = verdict(schema, deal(50), DIRECT); + expect(refused?.code).toBe('VALIDATION_FAILED'); + expect(refused?.fields).toEqual([expect.objectContaining({ field: '_record', code: 'rule_violation', message: OTHERWISE })]); + // `otherwise` admits what it does not refuse, and `then` is not run. + expect(verdict(schema, deal(50000), DIRECT)).toBeNull(); + }); + + it('a `when` nested in a branch is evaluated on the related row too', () => { + const nested = schemaOf({ + name: 'outer', type: 'conditional', severity: 'error', message: 'unused', when: 'record.amount > 0', + then: gate("record.account.type == 'partner'", 'inner'), + }); + expect(messagesOf(verdict(nested, deal(50000), PARTNER))).toEqual([THEN]); + expect(messagesOf(verdict(nested, deal(50), DIRECT))).toEqual([OTHERWISE]); + expect(verdict(nested, deal(50000), DIRECT)).toBeNull(); + }); + + it('does not hand the expanded row to the branch rules or back to the caller', () => { + // The branch reads the stored id: hydration is per predicate, never per rule tree. + const schemaWithIdBranch = schemaOf({ + ...gate("record.account.type == 'partner'"), + then: { name: 'blocked', type: 'script', severity: 'error', message: 'blocked account', condition: "record.account == 'acc_1'" }, + }); + const data = deal(10); + expect(messagesOf(verdict(schemaWithIdBranch, data, PARTNER))).toEqual(['blocked account']); + expect(data.account).toBe('acc_1'); + }); + + it('CONTROL — the guard spelling keeps reading the stored id', () => { + const guarded = schemaOf({ + name: 'linked', type: 'conditional', severity: 'error', message: 'unused', when: 'record.account != null', + then: { name: 'cap', type: 'script', severity: 'error', message: THEN, condition: "record.account.type == 'partner' && record.amount > 10000" }, + }); + expect(verdict(guarded, { name: 'A', amount: 50000, account: null }, {})).toBeNull(); + expect(messagesOf(verdict(guarded, deal(50000), PARTNER))).toEqual([THEN]); + }); + }); + + describe('a `when` the engine cannot serve has no verdict — and a sibling keeps its own', () => { + it('a second hop is refused as unevaluable, naming the rule; the sibling still judges', () => { + const schema = schemaOf(gate("record.account.owner.region == 'north'"), sibling); + const refused = verdict(schema, deal(200000), PARTNER); + expect(refused?.code).toBe('VALIDATION_FAILED'); + const gateError = refused?.fields.find((f) => f.constraint?.rule === 'partner_gate'); + expect(gateError).toMatchObject({ + field: '_record', + code: 'rule_violation', + constraint: { rule: 'partner_gate', reason: 'unevaluable', fault: 'reads more than one relationship hop' }, + }); + expect(gateError?.message).toMatch(/^Validation rule 'partner_gate' could not be evaluated \(reads more than one relationship hop\)/); + expect(messagesOf(refused)).toContain(SIBLING); + // Neither branch ran. + expect(messagesOf(refused).join('\n')).not.toContain('GATE-'); + // The sibling admits on its own: only the gate refuses. + expect(verdict(schema, deal(10), PARTNER)?.fields.map((f) => f.constraint?.rule)).toEqual(['partner_gate']); + }); + + it('a field the related object does not declare is refused, naming only that field; a sibling through the same reference keeps its verdict', () => { + const schema = schemaOf(gate("record.account.region == 'north'"), opportunity.validations[0]); + // The engine reads the declared rest and lists the undeclared column. + const related = { account: { ...row({ id: 'acc_1', type: 'partner' }), undeclaredFields: ['region'] } }; + const refused = verdict(schema, deal(50000), related); + expect(refused?.code).toBe('VALIDATION_FAILED'); + const gateError = refused?.fields.find((f) => f.constraint?.rule === 'partner_gate'); + expect(gateError).toMatchObject({ field: '_record', code: 'rule_violation', constraint: { rule: 'partner_gate', reason: 'unevaluable' } }); + expect(gateError?.message).toContain("'crm_account' declares no 'region'"); + // The sibling reading `type` through the same reference refuses with its own message… + expect(messagesOf(refused)).toContain('Partner accounts are capped at 10000.'); + // …and admits what it admits alone. + const direct = { account: { ...row({ id: 'acc_1', type: 'direct' }), undeclaredFields: ['region'] } }; + expect(verdict(schema, deal(50000), direct)?.fields.map((f) => f.constraint?.rule)).toEqual(['partner_gate']); + }); + + it('an empty reference is refused naming the guard, not as a fault in this object\'s columns', () => { + const refused = verdict(schemaOf(gate("record.account.type == 'partner'")), { name: 'A', amount: 50, account: null }, + { account: unavailable('no-reference') }); + expect(refused?.fields).toEqual([expect.objectContaining({ field: '_record', code: 'rule_violation', constraint: expect.objectContaining({ rule: 'partner_gate', reason: 'unevaluable' }) })]); + expect(refused?.fields[0]?.message).toContain('`conditional` rule whose `when` is `record.account != null`'); + expect(refused?.fields[0]?.message).not.toContain('which this object does not declare'); + }); + + it('a delete\'s reference cleanup is named, as it is for a `condition`', () => { + const refused = (() => { + try { + evaluateValidationRules( + { fields: { amount: { type: 'number' }, account: { type: 'lookup', reference: 'crm_account' } }, + validations: [gate("record.account.status == 'closed'")] } as any, + { amount: 50, account: null }, 'update', + { previous: { id: 'deal_1', amount: 50, account: null }, related: referentialClearBinding({ + object: 'crm_account', id: 'acc_1', referencingObject: 'crm_deal', field: 'account', + }) }, + ); + return null; + } catch (e) { + return e as ValidationError; + } + })(); + expect(refused).toBeInstanceOf(ValidationError); + expect(refused?.message).toContain('Cannot delete crm_account (acc_1): the delete clears `account`'); + expect(refused?.message).toContain("validation rule 'partner_gate' on crm_deal could not be evaluated"); + }); + }); +}); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 65c8a0d29e..9e8b657813 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -524,11 +524,14 @@ export function needsPriorRecord( * case and is what lets the engine skip the extra read entirely: the N+1 bound * is "one hop, only the named fields, only when a rule asks". * - * ## Scope: `script` / `cross_field`, including inside `conditional` - * - * These are the rules {@link checkPredicate} evaluates, and they are fail-CLOSED - * (#4649) — the one policy under which a rule that cannot be evaluated refuses - * the write instead of waving it through. The field-level `requiredWhen` / + * ## Scope: `script` / `cross_field`, and a `conditional`'s `when`, at any depth + * + * These are the predicates {@link checkPredicate} and {@link checkConditional} + * evaluate, and they are fail-CLOSED (#4649) — the one policy under which a + * rule that cannot be evaluated refuses the write instead of waving it + * through. A `when` is collected beside the rules it wraps (#24512): it is + * evaluated on the same per-predicate scope, so a `when` reading through a + * reference picks its branch on the related row. The field-level `requiredWhen` / * `readonlyWhen` / option `visibleWhen` predicates are deliberately NOT * collected here: this capability hydrates for a rule whose refusal is the * point, and the field level is not hydrated at all — a field predicate that @@ -557,11 +560,13 @@ export function collectPredicateRelationships( const visit = (rule: unknown, depth: number): void => { if (!rule || typeof rule !== 'object' || depth > 8) return; - const r = rule as { type?: unknown; condition?: unknown; then?: unknown; otherwise?: unknown }; + const r = rule as { type?: unknown; condition?: unknown; when?: unknown; then?: unknown; otherwise?: unknown }; if (r.type === 'script' || r.type === 'cross_field') addFrom(r.condition); - // A `conditional` wraps the rules it guards; its own `when` is evaluated - // by a different seam, so only the wrapped rules are collected here. + // [#24512] A `conditional`'s own `when` is a predicate `checkConditional` + // evaluates on the same hydrated scope `checkPredicate` gives a condition, + // so its hops are preloaded too — at every depth, as the wrapped rules are. if (r.type === 'conditional') { + addFrom(r.when); visit(r.then, depth + 1); visit(r.otherwise, depth + 1); } @@ -4453,6 +4458,34 @@ function referentialClearRefusal( }; } +/** + * [#18682 / #24512] The refusal a predicate gives when + * {@link resolveTraversalScope} decides it may not be evaluated: the rule HAS + * no verdict, so the write is rejected exactly as an unevaluable predicate's is + * (#4649), with the sentence naming the related object and field it could not + * read. One builder for both slots that are evaluated on that scope — a + * `script` / `cross_field` condition ({@link checkPredicate}) and a + * `conditional`'s `when` ({@link checkConditional}) — so the two cannot word + * or shape the same refusal differently. + */ +function traversalScopeRefusal( + ruleName: string, + field: string, + scope: Extract, + what: 'predicate' | 'when-predicate', + logger: EvaluateRulesOptions['logger'], +): FieldValidationError { + logger?.warn?.( + `Validation rule '${ruleName}' ${what} could not be evaluated (${scope.summary}) — write rejected`, + ); + return { + field, + code: 'rule_violation', + message: `Validation rule '${ruleName}' could not be evaluated (${scope.summary}) — write rejected.${scope.detail}`, + constraint: { rule: ruleName, reason: 'unevaluable', fault: scope.summary }, + }; +} + function checkPredicate( rule: PredicateRule, record: Record, @@ -4471,18 +4504,7 @@ function checkPredicate( // (#4649), but says which RELATED object and field it could not read. if (typeof expr.source === 'string' && expr.dialect === 'cel') { const scope = resolveTraversalScope(record, expr.source, related, fields); - if (!scope.ok) { - logger?.warn?.( - `Validation rule '${rule.name}' predicate could not be evaluated (${scope.summary}) — write rejected`, - ); - return { - field, - code: 'rule_violation', - message: - `Validation rule '${rule.name}' could not be evaluated (${scope.summary}) — write rejected.${scope.detail}`, - constraint: { rule: rule.name, reason: 'unevaluable', fault: scope.summary }, - }; - } + if (!scope.ok) return traversalScopeRefusal(rule.name, field, scope, 'predicate', logger); record = scope.record; } @@ -4649,23 +4671,45 @@ function checkJsonSchema( * 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). + * + * [#24512] `when` is evaluated on the scope {@link resolveTraversalScope} + * decides, exactly as {@link checkPredicate} evaluates a condition: hydrated + * with the related rows for the references THIS `when` reads through (the + * collector preloads them), or refused with the sentence naming what it could + * not read. Before, `when` met the bare foreign key, so `record.account.status` + * faulted and every write the rule guarded was refused. The hydrated copy is + * this predicate's alone: the branch is handed `ctx` untouched and decides its + * own scope, so a branch comparing the bare id keeps seeing the id. */ function checkConditional( rule: ConditionalRule, ctx: RuleContext, ): FieldValidationError | null { - const result = ExpressionEngine.evaluate(toExpression(rule.when), ruleScope(ctx.merged, ctx.previous)); + const expr = toExpression(rule.when); + let record = ctx.merged; + if (typeof expr.source === 'string' && expr.dialect === 'cel') { + const scope = resolveTraversalScope(record, expr.source, ctx.related, ctx.fields); + if (!scope.ok) return traversalScopeRefusal(rule.name, '_record', scope, 'when-predicate', ctx.logger); + record = scope.record; + } + const result = ExpressionEngine.evaluate(expr, ruleScope(record, ctx.previous)); if (!result.ok) { ctx.logger?.warn?.( `Validation rule '${rule.name}' when-predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — write rejected: a rule that cannot be evaluated fails closed, it is never skipped`, ); const nullRead = nullPreviousReadDetail(result.error, rule.when, ctx.previous, ctx.fields, 'when-predicate'); - return unevaluableRuleError( + const unevaluable = unevaluableRuleError( rule.name, '_record', result.error, 'when-predicate', nullRead !== undefined ? { prose: `Validation rule '${rule.name}'`, detail: nullRead } : undefined, declaredColumnRead(ctx.fields, celSourceOfPredicate(rule.when)), ); + // [#20006] Same verdict; on a delete's reference cleanup, a text that names + // it — the `when` reads through a reference as a condition does. + const onCleanup = expr.dialect === 'cel' + ? referentialClearRefusal(rule.name, expr, unevaluable, record, ctx.previous, ctx.related, ctx.fields) + : null; + return onCleanup ?? unevaluable; } const branch = result.value === true ? rule.then : rule.otherwise;