Skip to content

Parent-scoped requiredWhen is unenforced server-side — the same gap #4889 closed for readonlyWhen, one slot over #4977

Description

@xuyushun441-sys

Found while implementing #4889 (PR #4972). Filed unassigned, deliberately not fixed there — see "Why it was left" below.

Summary

#4889 closed the parent-scope hole for readonlyWhen: the server-side strip
now resolves the master-detail header and binds it as parent, so
readonlyWhen: P`parent.status == 'paid'` is enforced where it is documented
to be enforced.

requiredWhen sits on the same field, is evaluated by the same module,
and still has the hole.

Evidence

packages/objectql/src/validation/rule-validator.tsevaluateValidationRules,
the field-level conditional block (~L690, the requiredWhen branch):

const res = ExpressionEngine.evaluate< boolean >(toExpression(pred), {
  record: merged,
  previous,
  currentUser,
});
if (!res.ok) {
  opts.logger?.warn?.(`requiredWhen for '${name}' failed to evaluate — skipped`);
  // ... rule not applied
}

No parent. So a detail object declaring
requiredWhen: P`parent.status == 'sent'` — "once the header is Sent, every
line must carry a description" — evaluates in the inline grid (which binds
parent) and is a no-op on the server: the write is accepted with the field
empty.

Note this is the mirror of #4889's failure mode, not the same one:
readonlyWhen failing open wrote a field that should have been frozen;
requiredWhen failing open accepts a record that should have been rejected.
Both are declared ≠ enforced (PD #10), on the same declaration site.

No app in this repo authors a parent-scoped requiredWhen today, so this is a
latent gap rather than a live data defect — the reason it is a normal-priority
issue and not an urgent one.

What #4889 already built that this can reuse

  • resolveMasterDetailRelation()packages/objectql/src/master-detail.ts
    (exactly one master_detail relationship ⇒ that master is parent).
  • ObjectQL.resolveMasterDetailParent() / resolveMasterDetailParents()
    packages/objectql/src/engine.ts, single-id and batched-bulk header reads,
    payload-FK-first so a repoint is judged against the master it lands on.
  • hasParentScopedReadonlyWhenInPayload() — the AST-based "does this predicate
    read parent?" gate; the requiredWhen counterpart is the same helper over a
    different slot.
  • The build-time gate in packages/lint/src/validate-expressions.ts that rejects
    a parent-scoped predicate on an object with no single master.

So the plumbing is ~all present. What is NOT decided is the semantics question
below, which is why this was not a "while I'm here" change.

The decision this needs (do not guess it)

requiredWhen's current unevaluable policy is skip (fail-open) —
ADR-0058 D5's tier, and #4649 explicitly declined to change it for field
predicates. #4889 narrowed exactly one case for readonlyWhen (unbound scope
root ⇒ treat as LOCKED) and recorded it as an ADR-0058 addendum.

The equivalent narrowing for requiredWhen is not symmetric, and that is the
whole question:

  • "Locked" is the conservative answer for readonlyWhen — it preserves stored
    data and costs a caller one rejected field.
  • The conservative answer for requiredWhen is reject the write (422),
    which is a much louder failure and can block writes on records whose header is
    merely unreadable at that moment.

Options:

My reading: A + C is the sound first step (enforcement gap closed, authoring
mistake caught where it is cheap, no new way to 422 a legitimate write), with B
reserved for a deliberate decision by the maintainer alongside the next review of
ADR-0058 D5. But this is a fail-policy question on a public authoring contract,
so it wants a maintainer's ruling rather than an implementer's preference.

Why it was left out of #4972

Scope discipline. #4889's brief was readonlyWhen, and adding a parent
binding to evaluateValidationRules changes the outcome surface of a different
rule family in the same call (object-level script/cross_field predicates share
that evaluation site and have been fail-CLOSED since #4649, so binding a new root
there flips previously-rejected writes to accepted). That is a wider blast radius
than the issue authorized, and it deserves its own change with its own tests.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions