Skip to content

Commit e24bc37

Browse files
committed
fix(objectql): an unevaluable advisory validation rule logs one line saying the write proceeded
Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJ2vytxktPaEAmkEavY9KP
1 parent d68aab6 commit e24bc37

2 files changed

Lines changed: 246 additions & 12 deletions

File tree

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { runWithAdvisoryAggregation, type AdvisoryGroup } from '@objectstack/core';
5+
6+
import { evaluateValidationRules } from './rule-validator.js';
7+
import { ValidationError } from './record-validator.js';
8+
9+
// [#10126] Pay the first transform of these dist-resolved workspace deps at MODULE
10+
// LOAD, not inside a clocked `it()` body.
11+
import '@objectstack/spec';
12+
import '@objectstack/formula';
13+
14+
/**
15+
* [#24782] What the operator log says about a validation rule that could not be
16+
* EVALUATED, by severity.
17+
*
18+
* #4649 fails such a rule closed — but `severity` still governs blocking, so an
19+
* unevaluable `warning` / `info` rule never refuses the write. Until #24782 its
20+
* log said the opposite, twice: the predicate evaluator's fail-closed sentence
21+
* ("— write rejected: a rule that cannot be evaluated fails closed"), logged
22+
* before the rule's severity was consulted, and the advisory branch's per-write
23+
* line, which embedded the unevaluable violation's own message (ending
24+
* "— write rejected."). An operator reading the log for a disputed write was
25+
* told it was refused when it was stored.
26+
*
27+
* The pins:
28+
* - an unevaluable advisory rule logs exactly ONE line, naming the rule, its
29+
* severity and the fault, and saying the write proceeded — never "write
30+
* rejected", never "fails closed" — on each predicate slot: a `script`
31+
* condition, a `conditional`'s `when`, and the relationship-traversal refusal;
32+
* - the seed / boot aggregation (#13889) folds that same advisory text;
33+
* - CONTROL: an unevaluable `error` rule is refused exactly as before, and its
34+
* log still says the write was rejected and the rule fails closed.
35+
*/
36+
37+
const FAULTED_CONDITION = 'int(record.title) > 1';
38+
39+
const probe = (severity: 'error' | 'warning' | 'info') => ({
40+
name: 'qa_vr_probe',
41+
fields: { title: { type: 'text', label: 'Title' } },
42+
validations: [
43+
{ type: 'script', name: 'qa_unevaluable', severity, message: 'x', condition: FAULTED_CONDITION },
44+
],
45+
});
46+
47+
const conditionalProbe = (severity: 'error' | 'warning') => ({
48+
name: 'qa_vr_probe',
49+
fields: { title: { type: 'text', label: 'Title' } },
50+
validations: [
51+
{
52+
type: 'conditional',
53+
name: 'qa_unevaluable_when',
54+
severity,
55+
message: 'x',
56+
when: FAULTED_CONDITION,
57+
then: { type: 'script', name: 'qa_then', message: 'then', condition: 'false' },
58+
},
59+
],
60+
});
61+
62+
/** A rule reading one hop through a lookup whose parent row was unreadable. */
63+
const traversalProbe = (severity: 'error' | 'warning') => ({
64+
name: 'crm_opportunity',
65+
fields: {
66+
amount: { type: 'currency', label: 'Amount' },
67+
account: { type: 'lookup', reference: 'crm_account', label: 'Account' },
68+
},
69+
validations: [
70+
{
71+
type: 'script',
72+
name: 'partner_cap',
73+
severity,
74+
message: 'Partner accounts are capped at 10000.',
75+
fields: ['amount'],
76+
condition: "record.account.type == 'partner' && record.amount > 10000",
77+
},
78+
],
79+
});
80+
const unreadableAccount = { account: { object: 'crm_account', unavailable: 'unreadable' } };
81+
82+
function run(
83+
schema: unknown,
84+
data: Record<string, unknown>,
85+
related?: Record<string, unknown>,
86+
): { warns: string[]; thrown: unknown; returned: unknown } {
87+
const warns: string[] = [];
88+
let thrown: unknown;
89+
let returned: unknown;
90+
try {
91+
returned = evaluateValidationRules(schema as never, data, 'insert', {
92+
logger: { warn: (m: unknown) => { warns.push(String(m)); } },
93+
...(related ? { related: related as never } : {}),
94+
});
95+
} catch (err) {
96+
thrown = err;
97+
}
98+
return { warns, thrown, returned };
99+
}
100+
101+
function expectOneAdvisoryLine(warns: string[], rule: string, severity: string, fault: RegExp): void {
102+
expect(warns).toHaveLength(1);
103+
const [line] = warns;
104+
expect(line).toContain(`Validation rule '${rule}' (${severity}):`);
105+
expect(line).toMatch(fault);
106+
expect(line).toContain('write proceeded');
107+
expect(line).not.toMatch(/write rejected/);
108+
expect(line).not.toMatch(/fails closed/);
109+
}
110+
111+
describe('[#24782] an unevaluable ADVISORY rule logs one line, and it says the write proceeded', () => {
112+
it.each(['warning', 'info'] as const)('a `script` condition at severity %s', (severity) => {
113+
const { warns, thrown, returned } = run(probe(severity), { title: 'abc' });
114+
115+
expect(thrown).toBeUndefined();
116+
// #22726: an unevaluable rule has no verdict, so it is not an advisory hit.
117+
expect(returned).toEqual([]);
118+
expectOneAdvisoryLine(warns, 'qa_unevaluable', severity, /could not be evaluated \(.+\)/);
119+
});
120+
121+
it('a `conditional` whose `when` cannot be evaluated', () => {
122+
const { warns, thrown, returned } = run(conditionalProbe('warning'), { title: 'abc' });
123+
124+
expect(thrown).toBeUndefined();
125+
expect(returned).toEqual([]);
126+
expectOneAdvisoryLine(warns, 'qa_unevaluable_when', 'warning', /could not be evaluated \(.+\)/);
127+
});
128+
129+
it('the relationship-traversal refusal (the related row was unreadable)', () => {
130+
const { warns, thrown, returned } = run(
131+
traversalProbe('warning'), { amount: 20000, account: 'acc_1' }, unreadableAccount,
132+
);
133+
134+
expect(thrown).toBeUndefined();
135+
expect(returned).toEqual([]);
136+
expectOneAdvisoryLine(warns, 'partner_cap', 'warning', /could not be evaluated \(.+\)/);
137+
});
138+
139+
it('on the seed / boot path the hit folds into the summary with the same advisory text, and no per-row line', async () => {
140+
const warns: string[] = [];
141+
let groups: AdvisoryGroup[] = [];
142+
await runWithAdvisoryAggregation(async () => {
143+
evaluateValidationRules(probe('warning') as never, { title: 'abc' }, 'insert', {
144+
logger: { warn: (m: unknown) => { warns.push(String(m)); } },
145+
});
146+
}, (g) => { groups = g; });
147+
148+
expect(warns).toEqual([]);
149+
expect(groups).toHaveLength(1);
150+
expect(groups[0]).toMatchObject({ rule: 'qa_unevaluable', severity: 'warning', rows: 1 });
151+
expect(groups[0]!.message).toMatch(/could not be evaluated \(.+\) — write proceeded/);
152+
expect(groups[0]!.message).not.toMatch(/write rejected|fails closed/);
153+
});
154+
});
155+
156+
describe('[#24782] CONTROL — an unevaluable `error` rule is refused, and its log says so', () => {
157+
const refusalOf = (thrown: unknown) => {
158+
expect(thrown).toBeInstanceOf(ValidationError);
159+
return (thrown as ValidationError).fields;
160+
};
161+
162+
it('a `script` condition', () => {
163+
const { warns, thrown } = run(probe('error'), { title: 'abc' });
164+
165+
const fields = refusalOf(thrown);
166+
expect(fields).toHaveLength(1);
167+
expect(fields[0]).toMatchObject({
168+
field: '_record',
169+
code: 'rule_violation',
170+
constraint: { rule: 'qa_unevaluable', reason: 'unevaluable' },
171+
});
172+
expect(fields[0]!.message).toMatch(/^Validation rule 'qa_unevaluable' could not be evaluated \(.+\) — write rejected\./);
173+
expect(warns).toHaveLength(1);
174+
expect(warns[0]).toMatch(
175+
/^Validation rule 'qa_unevaluable' predicate failed to evaluate \(.+\) — write rejected: a rule that cannot be evaluated fails closed, it is never skipped$/,
176+
);
177+
});
178+
179+
it('a `conditional` whose `when` cannot be evaluated', () => {
180+
const { warns, thrown } = run(conditionalProbe('error'), { title: 'abc' });
181+
182+
expect(refusalOf(thrown)[0]).toMatchObject({ constraint: { rule: 'qa_unevaluable_when', reason: 'unevaluable' } });
183+
expect(warns).toHaveLength(1);
184+
expect(warns[0]).toMatch(/^Validation rule 'qa_unevaluable_when' when-predicate failed to evaluate \(.+\) — write rejected: a rule that cannot be evaluated fails closed/);
185+
});
186+
187+
it('the relationship-traversal refusal', () => {
188+
const { warns, thrown } = run(traversalProbe('error'), { amount: 20000, account: 'acc_1' }, unreadableAccount);
189+
190+
expect(refusalOf(thrown)[0]).toMatchObject({ constraint: { rule: 'partner_cap', reason: 'unevaluable' } });
191+
expect(warns).toHaveLength(1);
192+
expect(warns[0]).toMatch(/^Validation rule 'partner_cap' predicate could not be evaluated \(.+\) — write rejected$/);
193+
});
194+
});

‎packages/objectql/src/validation/rule-validator.ts‎

Lines changed: 52 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,24 @@ interface RuleContext {
339339
* when it resolved none. Applied per rule — see
340340
* {@link resolveTraversalScope}. */
341341
related: RelatedRecordBinding | undefined;
342+
/** [#24782] The TOP-LEVEL rule being evaluated is advisory (`warning` /
343+
* `info`) — set per rule by `evaluateValidationRules`, and inherited by a
344+
* `conditional`'s branch, whose outer rule's severity governs. See
345+
* {@link failClosedLogger}. */
346+
advisory: boolean;
347+
}
348+
349+
/**
350+
* [#24782] The logger an unevaluable predicate writes its fail-closed sentence
351+
* to ("— write rejected: a rule that cannot be evaluated fails closed"): the
352+
* write's logger for an `error` rule, whose write IS rejected, and none for an
353+
* advisory rule, whose write proceeds. The predicate evaluators run before the
354+
* caller acts on the rule's severity, so they cannot word the outcome
355+
* themselves; for an advisory rule `evaluateValidationRules` reports the fault
356+
* once, worded as what happened — see {@link unevaluableAdvisoryMessage}.
357+
*/
358+
function failClosedLogger(ctx: Pick<RuleContext, 'logger' | 'advisory'>): EvaluateRulesOptions['logger'] {
359+
return ctx.advisory ? undefined : ctx.logger;
342360
}
343361

344362
/**
@@ -3438,9 +3456,10 @@ export function nullPreviousReadDetail(
34383456
* `onValidationAdvisory` listener (see {@link emitValidationAdvisories}) and
34393457
* the validate-only preview appends it to the row's `warnings`. A rule that
34403458
* could not be EVALUATED (`constraint.reason: 'unevaluable'`) is not a hit: it
3441-
* has no verdict, and its text ("could not be evaluated … write rejected") is a
3442-
* fault report for the operator, false for a write an advisory never blocks —
3443-
* so it stays in the log only.
3459+
* has no verdict, and its text ("could not be evaluated … write rejected") is
3460+
* the refusal an `error` rule returns, false for a write an advisory never
3461+
* blocks — so it is not a hit, and the log reports it once in its own words
3462+
* ({@link unevaluableAdvisoryMessage}, #24782).
34443463
*/
34453464
export function evaluateValidationRules(
34463465
objectSchema: { validations?: unknown[]; fields?: Record<string, ConditionalFieldDef> } | undefined | null,
@@ -3478,7 +3497,7 @@ export function evaluateValidationRules(
34783497
// and update: what a predicate can read is the object's DECLARED shape, not
34793498
// whatever subset of columns this driver happened to return.
34803499
if (groundTruth) materializeDeclaredFields(merged, fields);
3481-
const ctx: RuleContext = { data, merged, previous, mode, logger: opts.logger, fields, messages: opts.messages, related: opts.related };
3500+
const ctx: RuleContext = { data, merged, previous, mode, logger: opts.logger, fields, messages: opts.messages, related: opts.related, advisory: false };
34823501

34833502
const errors: FieldValidationError[] = [];
34843503
// [#22726] The advisory hits this evaluation returns — see the doc above.
@@ -3603,9 +3622,10 @@ export function evaluateValidationRules(
36033622
.sort((a, b) => (a.priority ?? 100) - (b.priority ?? 100));
36043623

36053624
for (const rule of ordered) {
3625+
const severity = rule.severity ?? 'error';
36063626
let violation: FieldValidationError | null = null;
36073627
try {
3608-
violation = evaluateRule(rule, ctx);
3628+
violation = evaluateRule(rule, { ...ctx, advisory: severity !== 'error' });
36093629
} catch (err) {
36103630
// Defensive: a broken rule must never brick a write.
36113631
opts.logger?.warn?.(`Validation rule '${rule.name}' threw — skipped`, err);
@@ -3614,8 +3634,8 @@ export function evaluateValidationRules(
36143634

36153635
if (!violation) continue;
36163636

3617-
const severity = rule.severity ?? 'error';
3618-
if (severity !== 'error' && violation.constraint?.reason !== 'unevaluable') {
3637+
const unevaluable = violation.constraint?.reason === 'unevaluable';
3638+
if (severity !== 'error' && !unevaluable) {
36193639
advisories.push(advisoryEvent(rule, severity, violation));
36203640
}
36213641
if (severity === 'error') {
@@ -3628,16 +3648,22 @@ export function evaluateValidationRules(
36283648
// row count and example rows. Off that path no scope is active,
36293649
// `recordAdvisoryHit` returns false, and the per-write line below is
36303650
// emitted verbatim — an ordinary interactive write is untouched.
3651+
//
3652+
// [#24782] An UNEVALUABLE advisory rule is reported through the same two
3653+
// outlets, once, but not with its violation's message: that text is the
3654+
// refusal an `error` rule returns ("… — write rejected."), false for a
3655+
// write this rule does not block. The evaluator logged nothing for it
3656+
// (see `failClosedLogger`), so this is its only line.
36313657
!recordAdvisoryHit({
36323658
object: advisoryObjectName(objectSchema),
36333659
rule: String(rule.name ?? '(unnamed)'),
36343660
severity,
3635-
message: violation.message,
3661+
message: unevaluable ? unevaluableAdvisoryMessage(rule, violation) : violation.message,
36363662
recordRef: advisoryRecordRef(merged, data),
36373663
})
36383664
) {
36393665
opts.logger?.warn?.(
3640-
`Validation rule '${rule.name}' (${severity}): ${violation.message}`,
3666+
`Validation rule '${rule.name}' (${severity}): ${unevaluable ? unevaluableAdvisoryMessage(rule, violation) : violation.message}`,
36413667
);
36423668
}
36433669
}
@@ -3646,6 +3672,20 @@ export function evaluateValidationRules(
36463672
return advisories;
36473673
}
36483674

3675+
/**
3676+
* [#24782] What an advisory rule that could not be EVALUATED reports, in place
3677+
* of its violation's message: the fault, and that the write proceeded — the
3678+
* rule is advisory, so having no verdict does not block (#4649 fails closed on
3679+
* `severity: 'error'` only). Names the branch when the fault is a
3680+
* `conditional`'s branch rather than the rule itself.
3681+
*/
3682+
function unevaluableAdvisoryMessage(rule: BaseRule, violation: FieldValidationError): string {
3683+
const faulted = violation.constraint?.rule;
3684+
const subject = typeof faulted === 'string' && faulted !== rule.name ? `its branch '${faulted}'` : 'the rule';
3685+
return `${subject} could not be evaluated (${String(violation.constraint?.fault)})`
3686+
+ ' — write proceeded: the rule is advisory, so it was not applied to this write';
3687+
}
3688+
36493689
/**
36503690
* [#22726] One advisory hit as the event the write reports it through. Every
36513691
* member is copied from the evaluation's own verdict — the violation the rule
@@ -3753,7 +3793,7 @@ function evaluateRule(rule: BaseRule, ctx: RuleContext): FieldValidationError |
37533793
return checkStateMachine(rule as StateMachineRule, ctx.mode, ctx.data, ctx.previous, ctx);
37543794
case 'script':
37553795
case 'cross_field':
3756-
return checkPredicate(rule as PredicateRule, ctx.merged, ctx.previous, ctx.logger, ctx.messages, ctx.related, ctx.fields);
3796+
return checkPredicate(rule as PredicateRule, ctx.merged, ctx.previous, failClosedLogger(ctx), ctx.messages, ctx.related, ctx.fields);
37573797
case 'format':
37583798
return checkFormat(rule as FormatRule, ctx.data, ctx.logger, ctx.messages);
37593799
case 'json_schema':
@@ -4689,13 +4729,13 @@ function checkConditional(
46894729
let record = ctx.merged;
46904730
if (typeof expr.source === 'string' && expr.dialect === 'cel') {
46914731
const scope = resolveTraversalScope(record, expr.source, ctx.related, ctx.fields);
4692-
if (!scope.ok) return traversalScopeRefusal(rule.name, '_record', scope, 'when-predicate', ctx.logger);
4732+
if (!scope.ok) return traversalScopeRefusal(rule.name, '_record', scope, 'when-predicate', failClosedLogger(ctx));
46934733
record = scope.record;
46944734
}
46954735
const result = ExpressionEngine.evaluate<boolean>(expr, ruleScope(record, ctx.previous));
46964736

46974737
if (!result.ok) {
4698-
ctx.logger?.warn?.(
4738+
failClosedLogger(ctx)?.warn?.(
46994739
`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`,
47004740
);
47014741
const nullRead = nullPreviousReadDetail(result.error, rule.when, ctx.previous, ctx.fields, 'when-predicate');

0 commit comments

Comments
 (0)