MemberJunction / MemberJunction/MJ
Promote entity-validation plumbing onto ValidationResult and BaseEntity (AddError/Finalize, IsNewOrDirty, fail-closed async read helpers)
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 308
Description
## Summary
Writing `Validate()` / `ValidateAsync()` overrides on `BaseEntity` subclasses currently means
hand-rolling the same four or five pieces of plumbing every time. The plumbing is small, but it is
*subtly* easy to get wrong, and the failure modes are all silent — a rule that never runs, an error
that never surfaces on the form, or a save that is blocked by a warning.
MJ core already contains **42 hand-rolled instances across 18 files**, and they have drifted into
**three mutually-incompatible idioms** (detail below). This proposes promoting a small set of
helpers onto `ValidationResult` and `BaseEntity` so the plumbing is written once.
These were extracted while building a teaching app with 13 validation rules across 4 entities; the
duplication threshold was hit at 4 classes, which is exactly the signal that the shape belongs one
level up.
---
## Evidence: the idioms have already diverged in MJ core
`ValidationResult` is a bare data holder — `Success: boolean` and `Errors: ValidationErrorInfo[]`,
no methods ([`packages/MJGlobal/src/ValidationTypes.ts:42`](../blob/next/packages/MJGlobal/src/ValidationTypes.ts#L42)).
So every override invents its own way to close out the result:
| Idiom | Used by | Behaviour on a `Warning` |
|---|---|---|
| `result.Success = false` at each push | `MJDashboardEntityExtended`, `MJEntityFieldEntityExtended` | correct (warnings untouched) |
| `result.Success = result.Success && result.Errors.length === 0` | `MJAISkillPermissionEntityServer`, `MJUserRoutineEntityServer`, `MJUserRoutineRecipientEntityServer` | ❌ **blocks the save** |
| `result.Success = errors.every(e => e.Type !== ValidationErrorType.Failure)` | `MJAIAgentEntityServer` | correct |
The middle idiom is a latent defect rather than a live one: those three classes do not push warnings
themselves. But `super.Validate()` legitimately can — `MJAIAgentEntityServer:136`,
`MJAIAgentSessionBridgeEntityServer:170`, `MJMLTrainingPipelineEntityServer:173` and
`MJAICredentialBindingEntityExtended:64` all emit `ValidationErrorType.Warning` today, and companion
/ IS-A parent validation flows into the same `result`. The moment a warning reaches one of those
three classes, a valid save starts failing with a message the user is told is only a warning.
A single shared `Finalize()` removes the whole category.
---
## Proposed API
### 1. On `ValidationResult` (`@memberjunction/global`)
Zero new dependencies — this is the natural home.
```ts
export class ValidationResult {
Success: boolean = false;
Errors: ValidationErrorInfo[] = [];
/**
* Record a failure against a specific FIELD and mark the result unsuccessful.
*
* `Source` MUST be the field name: `mj-form-field` filters the result of `Record.Validate()`
* by it to decide which control renders the message. An error with the wrong Source still
* blocks the save but appears nowhere on the form — which reads to the user as a save that
* failed silently.
*/
AddError(source: string, message: string, value?: unknown): this;
/** Record a non-blocking warning. Does NOT change `Success`. */
AddWarning(source: string, message: string, value?: unknown): this;
/** True when at least one `Failure`-typed error is present. */
get HasFailures(): boolean;
/**
* Recompute `Success` from the errors actually present — `Failure`-typed only.
* Call at the end of an override. Idempotent.
*/
Finalize(): this;
}
```
### 2. On `BaseEntity` (`@memberjunction/core`)
```ts
/** True when the record is new, or any named field is dirty. The fast-path guard for a rule. */
public IsNewOrDirty(...fieldNames: string[]): boolean;
```
Every non-trivial rule opens with this, and the hand-rolled version
(`!this.IsSaved || fields.some(f => this.GetFieldByName(f)?.Dirty === true)`) is easy to write as
`this.Dirty`, which is wrong: it is true when *any* field changed, so an unrelated edit re-runs an
expensive async rule on every save.
### 3. Read helpers for `ValidateAsync()` — the substantive part
`ValidateAsync()` exists precisely for rules that need to read other records, and every such rule
needs the same three shapes. The important part is **not** the convenience, it is the return type:
```ts
export type EntityValidationRead = { ok: true; row: T | null } | { ok: false };
export type EntityValidationReadList = { ok: true; rows: T[] } | { ok: false };
export type EntityValidationCount = { ok: true; count: number } | { ok: false };
protected async ValidationReadOne(entityName: string, filter: string, fields: string[]): Promise>;
protected async ValidationReadMany(entityName: string, filter: string, fields: string[]): Promise>;
protected async ValidationCount(entityName: string, filter: string): Promise;
```
**Why the discriminated union is the whole point.** The obvious implementation returns `T | null`,
and then every call site writes:
```ts
const unit = await readOne('MJ: Housing Units', `ID='${id}'`);
if (!unit) return; // ← BUG: a FAILED read is silently treated as "no such row"
if (unit.Species !== this.Species) result.AddError(...);
```
A transient read failure — connection blip, permission change, timeout — makes the rule skip itself
and the row writes anyway. Nothing downstream catches it, because a foreign key knows nothing about
capacity, species, or vaccination history. Forcing the caller to distinguish `{ ok: false }` from
`{ ok: true, row: null }` makes that impossible to write by accident.
Paired with a fail-closed helper:
```ts
/**
* Fail closed when a rule could NOT be evaluated. Blocking a legitimate save is recoverable —
* the user retries and sees why. Writing an illegitimate one is not: the bad row is durable.
*/
protected MarkUnverified(result: ValidationResult, field: string, what: string): void;
```
`ValidationCount` is `MaxRows: 1` + `TotalRowCount` — it asks SQL to count and transfers one row,
rather than fetching N rows to call `.length` on them. `ValidationReadMany` exists for the case
where the rows themselves are needed (naming the offending records in the error message).
All three read `this.RunViewProviderToUse` and `this.ContextCurrentUser`, both already on
`BaseEntity`. Note `RunViewProviderToUse` and **not** `ProviderToUse` — the latter is an
`IEntityDataProvider` and does not satisfy `RunView`'s constructor. That mistake compiles in some
call shapes and is another thing a helper would stop people rediscovering.
### 4. Date-day comparison (`@memberjunction/global`) — lower confidence, include or drop
```ts
/** Reduce a value to a UTC day number, or null if absent/unparseable. */
export function ToUTCDayNumber(d: Date | string | null | undefined): number | null;
/** Today as a UTC day number. */
export function TodayUTCDayNumber(): number;
```
Every "not in the future" / "A must precede B" rule over a SQL `DATE` needs this. A SQL `DATE`
arrives as an instant at midnight UTC, so comparing it against a local `new Date()` makes the rule
depend on the browser's *time of day* as well as its date, and flip sign for anyone west of UTC —
the same root cause as #4210 in the read-mode formatter. Reducing both sides to a UTC day first
removes the question. This one is the most arguable of the four; it may belong in a date utility
rather than the validation surface.
---
## What this would look like at a call site
Before (current MJ, ~35 lines for two rules):
```ts
public override async ValidateAsync(): Promise {
const result = await super.ValidateAsync();
if (this.IsSaved && !this.GetFieldByName('HousingID')?.Dirty) return result;
if (!this.HousingID) return result;
const rv = new RunView(this.RunViewProviderToUse);
const res = await rv.RunView<{ Species: string; Capacity: number }>(
{ EntityName: 'MJ: Housing Units', ExtraFilter: `ID='${this.HousingID}'`,
Fields: ['Species', 'Capacity'], MaxRows: 1, ResultType: 'simple' },
this.ContextCurrentUser);
if (!res.Success) { /* …and here is where everyone writes `return result`… */ }
const unit = res.Results?.[0];
if (!unit) return result;
if (unit.Species !== this.Species) {
result.Errors.push(new ValidationErrorInfo('HousingID', '…', this.HousingID,
ValidationErrorType.Failure));
result.Success = false;
}
result.Success = result.Errors.length === 0; // ← and which idiom did we pick?
return result;
}
```
After:
```ts
public override async ValidateAsync(): Promise {
const result = await super.ValidateAsync();
if (!this.IsNewOrDirty('HousingID', 'Species') || !this.HousingID) return result;
const unit = await this.ValidationReadOne<{ Species: string; Capacity: number }>(
'MJ: Housing Units', `ID='${this.HousingID}'`, ['Species', 'Capacity']);
if (!unit.ok) this.MarkUnverified(result, 'HousingID', 'the housing unit');
else if (unit.row && unit.row.Species !== this.Species)
result.AddError('HousingID', `A ${this.Species} cannot be placed in a ${unit.row.Species} unit.`, this.HousingID);
return result.Finalize();
}
```
---
## Scope / migration
- Purely **additive**. No existing override changes behaviour; `Errors.push(...)` keeps working.
- The 18 existing hand-rolled files can be converted opportunistically. Converting the three
`Errors.length === 0` files is the one change with a real behavioural effect — it is the latent
warning bug above, and it is a fix.
- `AddError` / `AddWarning` / `Finalize` return `this` so they chain, but nothing requires it.
## Open questions for whoever picks this up
1. **`ValidationResult` methods vs. free functions.** Methods read better and are discoverable on
the object you already hold. The counter-argument is that `ValidationResult` is currently a pure
data shape that crosses the wire; adding methods means an object rehydrated from JSON loses them.
If `ValidationResult` is ever serialized and reconstituted with `Object.assign` / a spread,
methods break and free functions do not. Worth checking before committing to the shape.
2. **Where the read helpers live.** `protected` on `BaseEntity` is the ergonomic answer, but it
grows `BaseEntity`, which is already large. The alternative is exported free functions taking the
entity as the first argument — which is how they were originally written, precisely because each
entity must extend its own CodeGen-generated class and there is no common ancestor to hang
protected methods on. Free functions compose where inheritance cannot; on `BaseEntity` itself
that constraint does not apply, so the objection is only about class size.
3. **`ValidationReadMany` has no `MaxRows` cap.** A rule that reads an unbounded set to name
offenders can transfer a lot. Should it take a cap, or trust the rule author?
4. Whether the date helpers belong in this issue at all, or a separate date-utility one.
## Origin
Extracted from a MemberJunction teaching app (Harbor Street Animal Shelter, MJ Academy module 6 —
`BaseEntity` overrides), where 13 rules across 4 entities each needed the same plumbing. Every
helper above earned its place by being written wrong at least once first — the `if (!unit) return`
bug in §3 is the one that shipped and had to be found by review.
Contributor guide
Research direction
Start with packages/MJGlobal/src/ValidationTypes.ts and the BaseEntity implementation, then inspect the 18 hand-rolled validation files named in the issue. Resolve the serialization, helper placement, read-list cap, and date-utility questions before implementing; done means the shared APIs cover the described validation plumbing and the three warning-sensitive classes no longer use the length-based finalization idiom.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- backend, backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100