fix(formula,objectql): name the working guard for an optional lookup in both traversal refusals

.changeset/20007-optional-lookup-guard-prescription.md

@@ -0,0 +1,48 @@
+---
+'@objectstack/formula': patch
+'@objectstack/objectql': patch
+---
+
+fix(formula,objectql): the two refusals a traversing validation rule on an optional lookup meets now name the repairs that work — a `conditional` wrapper or `required: true` (#20007)
+
+Clause-②: no
+
+An author who wants to refuse a write when an OPTIONAL lookup is set and its related record is secret writes `record.line != null && record.line.kind == 'secret'`. Two refusals then sent them in a circle:
+
+1. That expression reads `line` both through the relationship and as a plain value, which cannot be served, and is refused. The refusal said to "compare the id explicitly" and write `record.line.id` for the value comparison.
+2. `record.line.id != null && record.line.kind == 'secret'` reads through `line` too, so an order with no line is refused before the rule is evaluated, as "no single related record". That refusal said to "guard the rule on the reference being set" and named no spelling for the guard.
+
+Which writes are refused is unchanged, and so are the error, the `rule_violation` field error and its `constraint` (`reason: 'unevaluable'` and the fault). `@objectstack/lint` passes the formula refusal through unchanged, so it shows the new text too. Only the prescriptions change. Both now name the two spellings measured to work for an optional reference, and the guard is worded exactly as in the delete-cleanup refusal:
+
+```text
+… To compare the id, write `record.line.id` for the value comparison, and keep
+`record.line.<related field>` for the traversal. `record.line.id` is not a null guard: it
+reads through `line` too, and a rule that reads through an empty `line` rejects the write
+instead of being skipped. If the plain value tests for empty, take that test out of this
+expression. To skip the rule while `line` is empty, guard it on `line` being set: make it
+the `then` of a `conditional` rule whose `when` is `record.line != null`. To refuse an
+empty `line`, make `line` required (`required: true`).
+```
+
+```text
+… A predicate resolves ONE hop through a single reference. To skip the rule while `line`
+is empty, guard it on `line` being set: make it the `then` of a `conditional` rule whose
+`when` is `record.line != null` — `record.line.id != null` inside the rule is no guard, as
+it reads through `line` too. To refuse an empty `line`, make `line` required
+(`required: true`). For a multi-value reference, test it with a macro (`exists`, `size`)
+instead of reading through it.
+```
+
+The repair as an author writes it, measured end to end on insert and update. It accepts an order with no line or a public line, and refuses a secret line with the rule's own message:
+
+```ts
+validations: [{
+  name: 'no_secret_line_when_set', type: 'conditional',
+  message: 'Only checked while the order names a line.',
+  when: 'record.line != null',
+  then: { name: 'no_secret_line', type: 'script', message: 'An order may not carry a secret line.',
+          condition: "record.line.kind == 'secret'" },
+}]
+```
+
+With `required: true` on `line` instead, an order with no line is refused at the field (`required`), and the rule still judges one with a line.

packages/formula/src/relationship-traversal.test.ts

@@ -121,6 +121,25 @@ describe('findTraversalConflicts — what the authoring layer refuses', () => {
     expect(conflicts[0].message).toContain('record.crm_account.id');
   });
 
+  // [#20007] The plain value is often a NULL TEST on an optional reference, and
+  // `.id` is no repair for that: it reads through the reference as well. The
+  // refusal must name the two spellings that do work — the guard in the ONE
+  // spelling ObjectQL's refusals use (held equal by objectql's
+  // `engine-predicate-relationship.test.ts`, which also drives both repairs end
+  // to end), and `required`.
+  it('names the guard and `required` for a null test on an optional reference', () => {
+    const a = analyzeRelationshipTraversals(
+      "record.crm_account != null && record.crm_account.type == 'partner'",
+    )!;
+    const [conflict] = findTraversalConflicts(a, isLookup);
+    expect(conflict.kind).toBe('bare-and-traversed');
+    expect(conflict.message).toContain('`record.crm_account.id` is not a null guard');
+    expect(conflict.message).toContain(
+      'make it the `then` of a `conditional` rule whose `when` is `record.crm_account != null`',
+    );
+    expect(conflict.message).toContain('make `crm_account` required (`required: true`)');
+  });
+
   // The short-circuit form is the one shape that can evaluate today for SOME
   // rows (the traversal is skipped when the left arm decides the verdict) and
   // fault for others. It is refused for that reason, not despite it.
@@ -189,6 +208,29 @@ describe('validateExpression — refuses the unserviceable traversal shapes', ()
     expect(message).toContain('record.account.id');
   });
 
+  // [#20007] The author-side refusal carries the same repairs as the engine's.
+  it('names the guard and `required` when the plain value is a null test', () => {
+    const r = validateExpression(
+      'predicate',
+      "record.account != null && record.account.type == 'partner'",
+      schema,
+    );
+    expect(r.ok).toBe(false);
+    const message = r.errors.map((e) => e.message).join('\n');
+    expect(message).toContain(
+      'make it the `then` of a `conditional` rule whose `when` is `record.account != null`',
+    );
+    expect(message).toContain('make `account` required (`required: true`)');
+  });
+
+  // …and the repair it names is accepted where the author writes it: the
+  // guard's `when` reads the reference only as a value, the wrapped condition
+  // only through it.
+  it('accepts both halves of the guarded rule', () => {
+    expect(validateExpression('predicate', 'record.account != null', schema).ok).toBe(true);
+    expect(validateExpression('predicate', "record.account.type == 'partner'", schema).ok).toBe(true);
+  });
+
   it('refuses a read deeper than one hop', () => {
     const r = validateExpression('predicate', 'record.account.owner.email != null', schema);
     expect(r.ok).toBe(false);

packages/formula/src/relationship-traversal.ts

@@ -221,6 +221,25 @@ export type TraversalConflictKind =
   /** Read through more than one hop; one hop is the declared depth. */
   | 'multi-hop';
 
+/**
+ * [#20007] The repair that guards a rule on a reference being set.
+ *
+ * ⭐ The same words as ObjectQL's `referenceGuardRepair` (`rule-validator.ts`),
+ * which its "no single related record" and delete-cleanup refusals carry: an
+ * author meets this sentence at authoring time here and at write time there,
+ * and two spellings of one repair read as two repairs. This package may not
+ * import ObjectQL, and exporting the wording from here would publish a sentence
+ * as API, so the two copies are held equal by a test instead:
+ * `packages/objectql/src/engine-predicate-relationship.test.ts` drives the
+ * engine's refusals from both sources and asserts one literal in each.
+ *
+ * Measured there end to end: the wrapped rule is skipped while the reference is
+ * empty and judged as before once it is set.
+ */
+function referenceGuardRepair(root: string, field: string): string {
+  return `make it the \`then\` of a \`conditional\` rule whose \`when\` is \`${root}.${field} != null\``;
+}
+
 /** One refusal-worthy finding about one field. */
 export interface TraversalConflict {
   readonly field: string;
@@ -250,6 +269,12 @@ export function findTraversalConflicts(
   for (const field of analysis.traversals.keys()) {
     if (!isReferenceField(field)) continue;
     if (!analysis.bareFields.has(field)) continue;
+    // [#20007] The plain value is often a NULL TEST on an optional reference
+    // (`record.line != null && record.line.kind == 'secret'`), and `.id` is no
+    // repair for that intent: it reads through the reference as well, so an
+    // empty reference still leaves the rule nothing to read and the write is
+    // rejected. So the id comparison is named for what it is, and the two
+    // spellings measured to work for the null test are named beside it.
     conflicts.push({
       field,
       kind: 'bare-and-traversed',
@@ -259,8 +284,14 @@ export function findTraversalConflicts(
         + `(\`${root}.${field}\`) in the same expression. Reading through the `
         + `relationship resolves \`${root}.${field}\` to the related RECORD, so the `
         + `plain-value comparison would stop matching the stored id — silently. `
-        + `Compare the id explicitly: write \`${root}.${field}.id\` for the value `
-        + `comparison, and keep \`${root}.${field}.<related field>\` for the traversal.`,
+        + `To compare the id, write \`${root}.${field}.id\` for the value `
+        + `comparison, and keep \`${root}.${field}.<related field>\` for the traversal. `
+        + `\`${root}.${field}.id\` is not a null guard: it reads through \`${field}\` `
+        + `too, and a rule that reads through an empty \`${field}\` rejects the write `
+        + `instead of being skipped. If the plain value tests for empty, take that test `
+        + `out of this expression. To skip the rule while \`${field}\` is empty, guard `
+        + `it on \`${field}\` being set: ${referenceGuardRepair(root, field)}. To refuse `
+        + `an empty \`${field}\`, make \`${field}\` required (\`required: true\`).`,
     });
   }
 

packages/objectql/src/engine-predicate-relationship.test.ts

@@ -735,3 +735,139 @@ describe('#20006 — a cascade reference clear refused by a traversing rule says
     }
   });
 });
+
+// ---------------------------------------------------------------------------
+// [#20007] An OPTIONAL lookup a traversing rule must skip while it is empty.
+//
+// The intent: refuse an order whose line is secret, and say nothing about an
+// order with no line. Both refusals on the way used to send the author in a
+// circle. The natural spelling, `record.line != null && record.line.kind ==
+// 'secret'`, is refused as a reference read both through the relationship and
+// as a value, and that refusal prescribed `record.line.id` — which is no null
+// guard: with `line` empty the engine refuses the rule before evaluation as
+// "no single related record", whose own prescription named no spelling at all.
+// The repairs that work are a `conditional` wrapper whose `when` is
+// `record.line != null`, or `required: true` on the lookup. Both refusals now
+// name them, in the ONE guard spelling the delete-cleanup refusal above uses,
+// and the wrapped rule is driven end to end here.
+// ---------------------------------------------------------------------------
+
+describe('#20007 — an optional lookup guarded in a traversing rule', () => {
+  const SECRET_MESSAGE = 'An order may not carry a secret line.';
+  /**
+   * The guard spelling every prescription names, byte for byte. ⭐ Step 1's
+   * refusal is worded by `@objectstack/formula` and step 2's by this package's
+   * `referenceGuardRepair`; formula may not import ObjectQL, so the words exist
+   * twice, and THIS literal asserted in both is what holds the copies equal.
+   */
+  const GUARD = 'make it the `then` of a `conditional` rule whose `when` is `record.line != null`';
+  const script = (condition: string) => ({
+    name: 'no_secret_line', type: 'script', severity: 'error', message: SECRET_MESSAGE, condition,
+  });
+  /** The natural spelling: a null test and a traversal in one expression. */
+  const natural = script("record.line != null && record.line.kind == 'secret'");
+  /** The spelling the mixed-shape refusal used to prescribe. */
+  const idTest = script("record.line.id != null && record.line.kind == 'secret'");
+  /** The repair, as an author writes it from the prescription. */
+  const wrapped = {
+    name: 'no_secret_line_when_set', type: 'conditional', severity: 'error',
+    message: 'Only checked while the order names a line.',
+    when: 'record.line != null', then: script("record.line.kind == 'secret'"),
+  };
+
+  async function boot(validations: unknown[], line: Record<string, unknown> = {}) {
+    const engine = new ObjectQL();
+    const d = makeDriver();
+    engine.registerDriver(d.driver, true);
+    await engine.init();
+    engine.registry.registerObject({
+      name: 'qa_line', fields: { name: { type: 'text' }, kind: { type: 'text' } },
+    } as any, 'test-package');
+    engine.registry.registerObject({
+      name: 'qa_order',
+      fields: {
+        name: { type: 'text' },
+        // OPTIONAL unless a case says otherwise.
+        line: { type: 'lookup', reference: 'qa_line', ...line },
+      },
+      validations,
+    } as any, 'test-package');
+    d.storeFor('qa_line').set('line_secret', { id: 'line_secret', name: 'S', kind: 'secret' });
+    d.storeFor('qa_line').set('line_public', { id: 'line_public', name: 'P', kind: 'public' });
+    const insert = (data: Record<string, unknown>) => engine
+      .insert('qa_order', { name: 'O', ...data }, { context: { isSystem: true } } as any)
+      .then(() => null, (e: unknown) => e as any);
+    return { engine, d, insert };
+  }
+
+  it('THE CIRCLE, step 1: the natural spelling is refused, and the refusal names the guard and `required`', async () => {
+    const { insert } = await boot([natural]);
+    const err = await insert({ line: 'line_public' });
+    expect(err?.code).toBe('VALIDATION_FAILED');
+    expect(err.fields).toHaveLength(1);
+    expect(err.fields[0]).toMatchObject({ field: '_record', code: 'rule_violation' });
+    expect(err.fields[0].constraint).toEqual({
+      rule: 'no_secret_line', reason: 'unevaluable',
+      fault: 'reads a reference field both through the relationship and as a value',
+    });
+    const message: string = err.message;
+    expect(message).toContain('`record.line.id`');     // still the id comparison
+    expect(message).toContain('not a null guard');     // …which it says is no guard
+    expect(message).toContain(GUARD);                  // repair 1
+    expect(message).toContain('make `line` required'); // repair 2
+    // ⛔ never the rule's own verdict: the rule was not evaluated.
+    expect(message).not.toContain(SECRET_MESSAGE);
+  });
+
+  it('THE CIRCLE, step 2: the `.id` spelling is no guard — an empty line is refused before evaluation, and that refusal names the guard too', async () => {
+    const { insert } = await boot([idTest]);
+    const err = await insert({});
+    expect(err?.code).toBe('VALIDATION_FAILED');
+    expect(err.fields).toHaveLength(1);
+    expect(err.fields[0]).toMatchObject({ field: '_record', code: 'rule_violation' });
+    expect(err.fields[0].constraint).toEqual({
+      rule: 'no_secret_line', reason: 'unevaluable',
+      fault: "cannot read 'id', 'kind' through `line` (object 'qa_line'): no single related record",
+    });
+    const message: string = err.message;
+    expect(message).toContain(GUARD);
+    expect(message).toContain('make `line` required');
+    expect(message).toContain('`record.line.id != null` inside the rule is no guard');
+    // CONTROL: the same rule is judged normally once the line is set.
+    expect(await insert({ line: 'line_public' })).toBe(null);
+    expect((await insert({ line: 'line_secret' }))?.message).toBe(SECRET_MESSAGE);
+  });
+
+  it('THE REPAIR: the wrapped rule ACCEPTS an empty line and a public one, and REFUSES a secret one with its own message', async () => {
+    const { insert, engine, d } = await boot([wrapped]);
+    expect(await insert({ id: 'o_empty' })).toBe(null);
+    expect(await insert({ id: 'o_null', line: null })).toBe(null);
+    expect(await insert({ id: 'o_public', line: 'line_public' })).toBe(null);
+    const err = await insert({ id: 'o_secret', line: 'line_secret' });
+    expect(err?.code).toBe('VALIDATION_FAILED');
+    expect(err.fields).toHaveLength(1);
+    // The nested rule's own verdict: a violation, not an unevaluable fault.
+    expect(err.fields[0]).toMatchObject({ field: '_record', code: 'rule_violation', message: SECRET_MESSAGE });
+    expect(err.fields[0].constraint?.reason).toBeUndefined();
+    expect(err.message).toBe(SECRET_MESSAGE);
+    expect(d.storeFor('qa_order').has('o_secret')).toBe(false);
+    // The UPDATE door: repointing an empty order at a secret line is refused,
+    // and emptying a set one is accepted.
+    const update = (id: string, patch: Record<string, unknown>) => engine
+      .update('qa_order', { id, ...patch }, { context: { isSystem: true } } as any)
+      .then(() => null, (e: unknown) => e as any);
+    expect((await update('o_empty', { line: 'line_secret' }))?.message).toBe(SECRET_MESSAGE);
+    expect(await update('o_public', { line: null })).toBe(null);
+    expect(d.storeFor('qa_order').get('o_public')?.line).toBe(null);
+  });
+
+  it('THE OTHER REPAIR: with `required: true` an empty line is refused at the FIELD, and a set one is judged by the rule', async () => {
+    const { insert } = await boot([script("record.line.kind == 'secret'")], { required: true });
+    const empty = await insert({});
+    expect(empty?.code).toBe('VALIDATION_FAILED');
+    // Only the field's own refusal: the rule is never reached on this write.
+    expect(empty.fields.map((f: any) => [f.field, f.code])).toEqual([['line', 'required']]);
+    expect(await insert({ line: 'line_public' })).toBe(null);
+    expect((await insert({ line: 'line_secret' }))?.message).toBe(SECRET_MESSAGE);
+  });
+});

packages/objectql/src/validation/rule-relationship-traversal.test.ts

@@ -248,6 +248,8 @@ describe('#18682 — the engine refuses the unserviceable shape, not only lint',
       const detail = JSON.stringify((e as unknown as { errors?: unknown }).errors ?? (e as Error).message);
       expect(detail).toContain('could not be evaluated');
       expect(detail).toContain('record.account.id');
+      // [#20007] …and, for a null test, the guard the engine's other refusals name.
+      expect(detail).toContain('`conditional` rule whose `when` is `record.account != null`');
       // ⛔ and never the rule's own message — the rule produced NO verdict.
       expect(detail).not.toContain('should never be reached');
     }
@@ -272,7 +274,12 @@ describe('#18682 — the refusal names the RELATED object, not the referencing o
   const cases: Array<[string, ReturnType<typeof unavailable>, string[]]> = [
     ['read failed', unavailable('unreadable'), ['could not read', "'crm_account'"]],
     ['undeclared related field', unavailable('undeclared-field', ['type']), ['declares no', "'type'"]],
-    ['no reference stored', unavailable('no-reference'), ['no single related record', 'MULTIPLE references']],
+    // [#20007] …and the two repairs measured to work for an EMPTY reference: the
+    // guard in `referenceGuardRepair`'s spelling, and `required`.
+    ['no reference stored', unavailable('no-reference'), [
+      'no single related record', 'MULTIPLE references',
+      '`conditional` rule whose `when` is `record.account != null`', 'make `account` required',
+    ]],
     ['related record not found', unavailable('unresolved'), ["'crm_account'", 'the related record was not found']],
   ];