fix(formula)!: the unknown-field check judges every member spelling of record / previous, not only the dot (#22428)

.changeset/22428-field-existence-every-spelling.md

@@ -0,0 +1,29 @@
+---
+"@objectstack/formula": minor
+---
+
+fix(formula)!: the unknown-field check judges every spelling of a `record` / `previous` member, not only the dot, so `os build` and the object save door refuse `record['typo']`, `previous['typo']`, `record.?typo` and `record[?'typo']` (#22428)
+
+Clause-②: yes (narrowing)
+
+<!-- adr-0087: not-required (no-migration-prescription) A refusal at `os build` and at the object save door of an expression that names an undeclared field in a non-dot member spelling: no authorable key, spelling, export or stored shape moves, and no stored row is read, rewritten or converted. A stored object whose expression is refused keeps loading until it is next saved, and the repair is the author's own (declare the field or fix the typo), which no ledger entry can derive. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers this verdict and this diff adds none (not registered / already-registered); and no exported TypeScript declaration changes (not runtime-interface-only / type-surface-only). -->
+
+**BREAKING**: an accept-set change, in both directions, of the `os build`, `os validate` and `os lint` verdicts and of the object save door, shipped as `minor` under the launch-window convention for accept-set narrowings. It narrows: an expression that names an undeclared field of its object in a non-dot member spelling (`record['FIELD']`, `record["FIELD"]`, `record.?FIELD`, `record[?'FIELD']`, or any of those on `previous`), or an undeclared leaf of a declared read attachment in such a spelling, is now refused where it was accepted. It widens: three shapes the dot-only regex misread as a member of the root, and refused as unknown fields, are now accepted, because none of them names a member. They are text inside a string literal (`record.name == 'record.typo'`), a root name after another root (`vars.record.x`), and a method call on the root itself (`record.size()`). No refusal code is added.
+
+`validateExpression`'s field-existence pass judges each member an expression reads on `record` or `previous` against the object's fields, and refuses an undeclared one with the `unknown-field` code. It found those members with a regex over the dot spelling, so it judged `record.typo` and `has(record.typo)` and nothing else. `record['typo']`, `record["typo"]`, `previous['typo']`, `record.?typo` and `record[?'typo']` name the same column, and they reached the evaluator with no verdict at every slot the pass judges, among them a select option's `visibleWhen`, a field's `requiredWhen` and a validation rule's condition. There the expression faults on a key the record never carries, or reads a value that never exists.
+
+The pass now reads members through the same AST member reader the relationship-traversal analysis is built on, so every spelling gets the dot spelling's verdict, did-you-mean included:
+
+- `os build`, `os validate` and `os lint` refuse an expression that names an undeclared field of its object as `record['FIELD']`, `record["FIELD"]`, `record.?FIELD`, `record[?'FIELD']` or any of those on `previous`, at `error`, with the message they already give for `record.FIELD`.
+- The object save door runs the same pass, so an object write in publish mode that carries such an expression is now refused with an `expression-invalid` issue located at the slot, the build's own finding.
+- A declared read attachment's leaf (`ObjectSchema.attachedOnRead`) is judged in the same spellings: `record.viewer['can_actt']` and `record.viewer.?can_actt` are refused like `record.viewer.can_actt`. An earlier entry in this release says index access on a read attachment stays unjudged; that describes the check before this change.
+
+**Remedy.** Declare the field on the object, or fix the typo to the field the message suggests. The spelling itself is not the defect: `record['status']`, `record.?status` and `record[?'status']` on a declared `status` are accepted, as before.
+
+**Unchanged.**
+
+- A computed key (`record[record.kind]`, `record[someVar]`) names no member before evaluation, so it is not judged.
+- A method call on a member (`record.name.startsWith('A')`) still judges the member (`name`).
+- Stored rows are not migrated or refused on read; an object stored before this change keeps loading until it is next saved, and that save is judged. Drafts are not gated.
+- Measured before crossing: the stacks this repository ships give the same expression findings before and after the change. That covers `examples/app-todo`, `examples/app-crm`, both `examples/app-multi-package` sub-stacks, the objects, actions, flows, views and pages of `examples/app-showcase`, the 51 objects `@objectstack/platform-objects` exports, and `plugin-approvals`' `sys_approval_request`, whose eight action predicates read the leaves of its declared `viewer` read attachment and pass. No in-repo producer writes a non-dot spelling of an undeclared field.
+- No public export or signature moves; only the doc comments of `ExprSchemaHint.fields` and `ExprSchemaHint.attachedOnRead` change. `analyzeRelationshipTraversals` answers exactly what it answered before: measured identical, set order included, on 4,528 analyses of the 2,264 `record` / `previous` expressions in this repository's TypeScript sources.

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

@@ -2,9 +2,12 @@
 
 import { describe, expect, it } from 'vitest';
 
+import { parseCelToAst } from './cel-engine';
 import {
   analyzeRelationshipTraversals,
   findTraversalConflicts,
+  readRootMembers,
+  traversalsOf,
 } from './relationship-traversal';
 import { validateExpression } from './validate';
 
@@ -108,6 +111,48 @@ describe('analyzeRelationshipTraversals — which hops an expression names', ()
   });
 });
 
+describe('readRootMembers — the one member reader both analyses fold', () => {
+  const readsOf = (source: string, roots: readonly string[]) => {
+    const ast = parseCelToAst(source);
+    expect(ast, source).not.toBeNull();
+    return readRootMembers(ast!, roots);
+  };
+
+  it('reads every member spelling of several roots in one walk, in source order', () => {
+    expect(readsOf(
+      "record['a'] == 1 && previous.?b.orValue(0) == 2 && has(record.c) && record[?'d'].orValue(0) == 3",
+      ['record', 'previous'],
+    )).toEqual([
+      { root: 'record', field: 'a', deeper: false },
+      { root: 'previous', field: 'b', deeper: false },
+      { root: 'record', field: 'c', deeper: false },
+      { root: 'record', field: 'd', deeper: false },
+    ]);
+  });
+
+  it('carries the next segment and whether the read goes deeper', () => {
+    expect(readsOf("record.a['b'] == 1 && record.c.?d.e == 2", ['record'])).toEqual([
+      { root: 'record', field: 'a', leaf: 'b', deeper: false },
+      { root: 'record', field: 'c', leaf: 'd', deeper: true },
+    ]);
+  });
+
+  it('reads no member for a computed key, a method on the root, or a root name in member position', () => {
+    expect(readsOf('record[k] == 1 && record.size() > 0 && vars.record.x == 1', ['record'])).toEqual([]);
+  });
+
+  it('a method call on a member uses the member as a value — no leaf', () => {
+    expect(readsOf("record.name.startsWith('A')", ['record'])).toEqual([
+      { root: 'record', field: 'name', deeper: false },
+    ]);
+  });
+
+  it('analyzeRelationshipTraversals is exactly the fold of its root\'s reads', () => {
+    const source = "record.crm_account.type == 'x' && record.crm_account == 'acc_1' && record.owner.team.lead == 'y'";
+    expect(analyzeRelationshipTraversals(source)).toEqual(traversalsOf(readsOf(source, ['record'])));
+  });
+});
+
 describe('findTraversalConflicts — what the authoring layer refuses', () => {
   it('refuses a FK that is both traversed and compared bare', () => {
     const a = analyzeRelationshipTraversals(

packages/formula/src/relationship-traversal.ts

@@ -45,7 +45,7 @@
  * is reported as a conflict rather than resolved by a precedence rule.
  */
 
-import { parseCelToAst } from './cel-engine';
+import { parseCelToAst, type CelAstNode } from './cel-engine';
 
 /** The default scope root a record-scoped predicate traverses from. */
 export const DEFAULT_TRAVERSAL_ROOT = 'record';
@@ -75,13 +75,6 @@ export interface RelationshipTraversalAnalysis {
   readonly multiHopFields: ReadonlySet<string>;
 }
 
-/** `{ op: 'id', args: '<root>' }` — a bare identifier node for `root`. */
-function isRootId(node: unknown, root: string): boolean {
-  if (!node || typeof node !== 'object') return false;
-  const { op, args } = node as { op?: unknown; args?: unknown };
-  return op === 'id' && args === root;
-}
-
 /**
  * A member access on `node`, whatever spelling it was written in, or `null`.
  *
@@ -132,6 +125,98 @@ function asMember(node: unknown): { receiver: unknown; name: string } | null {
   return null;
 }
 
+/**
+ * One read of a member of a named scope root, as {@link readRootMembers}
+ * reports it — whatever spelling the author wrote it in.
+ */
+export interface RootMemberRead {
+  /** The scope root the read starts from — `record`, `previous`, … */
+  readonly root: string;
+  /** The member read on the root: `crm_account` in `record['crm_account'].type`. */
+  readonly field: string;
+  /**
+   * The member read in turn on `root.<field>`, when the expression reads one:
+   * `type` in `record.crm_account.?type`. Absent when `root.<field>` is used
+   * as a value — compared, handed to a function, or the receiver of a METHOD
+   * call (`record.name.startsWith('A')`), which reads the value, not a member.
+   */
+  readonly leaf?: string;
+  /** The read continues past `leaf` (`record.a.b.c`): more than one hop. */
+  readonly deeper: boolean;
+}
+
+/**
+ * Every read of a member of one of `roots` in a parsed CEL expression, in every
+ * member spelling {@link asMember} recognises — `.`, `.?`, `['…']` and
+ * `[?'…']` — in source order, one entry per occurrence.
+ *
+ * ⭐ The ONE member reader in this package. Two questions are answered from it
+ * and nothing else: which hops an expression takes through a root
+ * ({@link analyzeRelationshipTraversals}), and whether each member it names
+ * exists (`validateExpression`'s field-existence pass). One reader means the
+ * two can never disagree about which spellings name a member — a spelling one
+ * of them missed is exactly how a typo used to pass the existence check while
+ * the traversal check saw it.
+ *
+ * What is NOT a read, by construction:
+ *  - an index whose key is not a string literal (`record[someVar]`) — which
+ *    member it names is not knowable before evaluation, so there is nothing to
+ *    judge, and a check built on this reader gives no verdict for it;
+ *  - a method call on the root itself (`record.size()`), which reads the root,
+ *    not a member of it;
+ *  - a root name in member position (`vars.record.x`) — only a bare
+ *    identifier is a root.
+ *
+ * Takes the AST rather than the source so a caller asking both questions of
+ * one expression parses it once. Parse it with {@link parseCelToAst}, the
+ * platform's one answer to "what parses".
+ */
+export function readRootMembers(ast: CelAstNode, roots: readonly string[]): RootMemberRead[] {
+  const rootSet = new Set(roots);
+  const reads: RootMemberRead[] = [];
+
+  // `{ op: 'id', args: '<root>' }` — a bare identifier node naming one of `roots`.
+  const rootNameOf = (node: unknown): string | undefined => {
+    if (!node || typeof node !== 'object') return undefined;
+    const { op, args } = node as { op?: unknown; args?: unknown };
+    return op === 'id' && typeof args === 'string' && rootSet.has(args) ? args : undefined;
+  };
+
+  // `via` is the member access this node is the RECEIVER of, handed down only
+  // along a receiver edge: its name is the next segment, and `deeper` says
+  // whether that access is itself read through in turn.
+  const walk = (node: unknown, via?: { name: string; deeper: boolean }): void => {
+    if (Array.isArray(node)) {
+      for (const child of node) walk(child);
+      return;
+    }
+    if (!node || typeof node !== 'object') return;
+
+    const member = asMember(node);
+    if (member) {
+      const root = rootNameOf(member.receiver);
+      if (root !== undefined) {
+        reads.push({
+          root,
+          field: member.name,
+          ...(via ? { leaf: via.name } : {}),
+          deeper: via?.deeper ?? false,
+        });
+        return;
+      }
+      // The receiver is read through this access. The access's other operand
+      // is its member name or a string-literal key, neither of which can hold
+      // a read.
+      walk(member.receiver, { name: member.name, deeper: via !== undefined });
+      return;
+    }
+
+    for (const value of Object.values(node as Record<string, unknown>)) walk(value);
+  };
+  walk(ast);
+  return reads;
+}
+
 /**
  * Analyse one authored CEL source for the hops it takes through `root`.
  *
@@ -152,61 +237,30 @@ export function analyzeRelationshipTraversals(
 ): RelationshipTraversalAnalysis | null {
   const ast = parseCelToAst(source);
   if (ast == null) return null;
+  return traversalsOf(readRootMembers(ast, [root]));
+}
 
+/**
+ * Fold one root's {@link readRootMembers} into the traversal analysis: a read
+ * with a `leaf` is a hop through `field` (and more than one when it goes
+ * `deeper`), a read without one uses `field` as a value.
+ */
+export function traversalsOf(reads: readonly RootMemberRead[]): RelationshipTraversalAnalysis {
   const traversals = new Map<string, Set<string>>();
   const bareFields = new Set<string>();
   const multiHopFields = new Set<string>();
-
-  // Member-access nodes reached AS THE RECEIVER of another member access are
-  // traversals, not values. Collect those first so the value pass can exclude
-  // them by identity rather than by re-deriving the shape.
-  const traversalReceivers = new Set<unknown>();
-
-  const walk = (node: unknown): void => {
-    if (Array.isArray(node)) {
-      for (const child of node) walk(child);
-      return;
-    }
-    if (!node || typeof node !== 'object') return;
-
-    const outer = asMember(node);
-    if (outer) {
-      const inner = asMember(outer.receiver);
-      if (inner && isRootId(inner.receiver, root)) {
-        // root.<inner.name>.<outer.name> — one hop through `inner.name`.
-        traversalReceivers.add(outer.receiver);
-        let fields = traversals.get(inner.name);
-        if (!fields) traversals.set(inner.name, (fields = new Set()));
-        fields.add(outer.name);
-      } else if (inner) {
-        // Deeper than one hop: root.a.b.c reaches here as (root.a.b).c, whose
-        // own receiver is itself a traversal. Attribute it to the FIRST field
-        // so the refusal can name what the author wrote.
-        const base = asMember(inner.receiver);
-        if (base && isRootId(base.receiver, root)) multiHopFields.add(base.name);
-      }
+  for (const read of reads) {
+    if (read.leaf === undefined) {
+      bareFields.add(read.field);
+      continue;
     }
-
-    for (const value of Object.values(node as Record<string, unknown>)) walk(value);
-  };
-  walk(ast);
-
-  // Second pass: every `root.<field>` node that was NOT consumed as a traversal
-  // receiver is a value use.
-  const walkValues = (node: unknown): void => {
-    if (Array.isArray(node)) {
-      for (const child of node) walkValues(child);
-      return;
-    }
-    if (!node || typeof node !== 'object') return;
-    const member = asMember(node);
-    if (member && isRootId(member.receiver, root) && !traversalReceivers.has(node)) {
-      bareFields.add(member.name);
-    }
-    for (const value of Object.values(node as Record<string, unknown>)) walkValues(value);
-  };
-  walkValues(ast);
-
+    let fields = traversals.get(read.field);
+    if (!fields) traversals.set(read.field, (fields = new Set()));
+    fields.add(read.leaf);
+    // Deeper than one hop: `root.a.b.c`. Attributed to the FIRST field so the
+    // refusal can name what the author wrote.
+    if (read.deeper) multiHopFields.add(read.field);
+  }
   return { traversals, bareFields, multiHopFields };
 }
 

packages/formula/src/validate-attached-on-read.test.ts

@@ -119,8 +119,40 @@ describe('ExprSchemaHint.attachedOnRead — record.<block>.<leaf>', () => {
     expect(refusalsOf('record.submitter_id.nmae == "x"', DECLARED)).toEqual([]);
   });
 
-  it('leaves a method call, an index read and a third segment unjudged — missed catches, never false refusals', () => {
-    expect(refusalsOf("record.viewer['can_actt'] == true", DECLARED)).toEqual([]);
+  it('judges the leaf in every member spelling, as it judges the dot', () => {
+    const refused = [{
+      code: 'unknown-field',
+      params: {
+        field: 'viewer.can_actt',
+        objectName: 'sys_approval_request',
+        suggestion: 'viewer.can_act',
+        block: 'viewer',
+        leaves: ['can_act', 'can_override', 'is_submitter'],
+      },
+    }];
+    for (const source of [
+      "record.viewer['can_actt'] == true",
+      "record.viewer.?can_actt.orValue(false) == true",
+      "record.viewer[?'can_actt'].orValue(false) == true",
+      "record['viewer'].can_actt == true",
+      "previous.?viewer.can_actt.orValue(false) == true",
+      'has(record.viewer.can_actt)',
+    ]) {
+      expect(refusalsOf(source, DECLARED), source).toEqual(refused);
+    }
+    // Control: the declared leaf in the same spellings.
+    for (const source of [
+      "record.viewer['can_act'] == true",
+      "record.viewer.?can_act.orValue(false) == true",
+      "record['viewer'][?'can_act'].orValue(false) == true",
+      'has(record.viewer.can_act)',
+    ]) {
+      expect(refusalsOf(source, DECLARED), source).toEqual([]);
+    }
+  });
+
+  it('leaves a computed key, a method call and a third segment unjudged — missed catches, never false refusals', () => {
+    expect(refusalsOf('record.viewer[record.status] == true', DECLARED)).toEqual([]);
     expect(refusalsOf("record.viewer.split(',') == []", DECLARED)).toEqual([]);
     expect(refusalsOf('record.viewer.can_act.foo == true', DECLARED)).toEqual([]);
   });