MemberJunction / MemberJunction/MJ

BaseEntity has no delete-validation seam — every app hand-rolls Delete() overrides to explain a refusal

Open
#3,971 1 comment 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

## Summary

`BaseEntity` has a save-validation seam — `Validate()` and `ValidateAsync()` — with a well-defined contract: return a `ValidationResult`, let the framework decide, get field-named errors in front of the user. **There is no equivalent for delete.** No `ValidateDelete()`, no `CanDelete()`, no `BeforeDelete` event that can veto.

So the only way to refuse a delete with an explanation is to override `Delete()` itself (`baseEntity.ts:4267`), check by hand, and return `false` before calling `super.Delete()`.

## Why that is worse than it sounds

**1. The message is lost.** `Validate()` returns `ValidationResult` with `ValidationErrorInfo[]`, each carrying a `Source` — which is how MJ's forms mark the offending *field*. A `Delete()` override returns `boolean`. There is nowhere to put "this template is referenced by 5 signed contracts"; the caller gets `false` and has to invent its own explanation, or the user gets whatever the raw FK error says.

**2. Every app reimplements it, differently.** In `bizapps-accounting` alone:

- `JournalEntryTypeEntityServer.Delete()`
- `JournalEntryLineEntityServer.Delete()`

…each with its own probe, its own logging, its own way of surfacing the reason. `bizapps-contracts` is about to add four more for the same reason (a provision referenced by modifications, a template referenced by signed contracts, a contract with lineage children, a type in use). That is six hand-rolled versions of a hook that should exist once.

**3. The guarantees `Validate()` gets, delete does not.** A save runs validation inside the framework's flow, so every caller — form, `SaveEntityGraph`, an agent, a Remote Operation — is covered by one rule. A `Delete()` override only covers callers that go through that subclass's method, and there is no framework guarantee about ordering relative to permissions, Record Changes, or soft-delete handling.

**4. Referential refusal is the common case, and it is always ugly.** Where FKs are `NO_ACTION` (the correct default), the database already refuses the delete — correctly. What is missing is the layer that turns `The DELETE statement conflicted with the REFERENCE constraint "FK_..."` into a sentence a user can act on. Today every app writes that layer by hand or ships the raw error.

## Proposed

A delete-side counterpart to the save seam, symmetrical with what already exists:

```typescript
// Called by BaseEntity.Delete() before any provider work, exactly as Validate()
// is called by Save(). A non-Success result aborts the delete.
public ValidateDelete(): ValidationResult
public async ValidateDeleteAsync(): Promise
```

Ideally with the same conventions the save side already has: `ValidationErrorInfo.Source` naming the relationship or field that blocks it, `ValidationErrorType` so a warning can be distinguished from a refusal, and the async variant opt-in the way `ValidateAsync` is (only run when a subclass overrides it, per the `DefaultSkipAsyncValidation` note in `baseEntity.ts`).

Two smaller alternatives, if a new virtual is unwelcome:

- **A vetoable event.** `Delete()` already raises events; making a `before_delete` event cancellable with a reason would cover most of this without new API surface.
- **Metadata-driven refusal.** MJ knows every FK to a row (`EntityRelationship`). A generic "this row is referenced by N `` records" message, derived from metadata and emitted by `Delete()` when the provider reports an FK violation, would fix the *common* case with no per-app code at all — and would arguably be the higher-value change, since it needs no subclass.

Happy to be told a hook exists and we missed it — we looked for `ValidateDelete`, `CanDelete` and `BeforeDelete` in `packages/MJCore/src/generic/baseEntity.ts` and found none.

## Environment

MJ `6.1.0-edge.2`. Observed while auditing validation coverage in `bizapps-contracts`; the accounting overrides above are the existing prior art.

Contributor guide

Open the contributing guide

Research direction

Read packages/MJCore/src/generic/baseEntity.ts around Delete() at line 4267, then compare the existing Validate()/ValidateAsync() seam. Review the Delete() overrides named in bizapps-accounting and the proposed alternatives; done requires a decided delete-validation approach with its caller coverage and user-facing refusal behavior defined.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend-api-design
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.