mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
knowledge(data-modeling): TableRelation delete/rename asymmetry and the xRec before-image contract
Three related concerns, each proven by executable tests rather than recall. They were extracted from a real defect that shipped through a six-reviewer panel undetected, which is the admission test passing on behavior rather than on theory. owning-table-must-delete-dependents-in-ondelete AL has no cascading delete. What makes it missable is an asymmetry: the platform DOES keep references correct on rename via TableRelation, so a developer who learns that and generalizes it to delete ships orphans. Also notes the permission trap (delete rights needed on the dependent table, not just the parent). validate-table-relation-false-suppresses-rename-propagation The non-obvious half. The property name implies input validation only, but disabling it also switches off rename propagation. Verified against a parent renamed once while a child held three fields: a normal relation (follows), the same relation with validation disabled (does NOT follow), and a field with no relation at all (does not follow) — the third being the control that proves the test can detect a non-propagating field. xrec-is-a-before-image-only-in-some-triggers Corrects both the naive belief that xRec is always the previous record and the folk rule that it 'only works from a page'. The behavior is per-trigger: a genuine before-image in OnRename and OnDelete regardless of driver, a mirror of Rec in OnInsert/OnModify when driven from code, and a real before-image in those two only when a page drove the write. That last asymmetry is why an OnModify comparison against xRec passes manual page testing and silently no-ops in a job queue. Targets /community per CONTRIBUTING — general BC knowledge, not fork-specific. Frontmatter validator clean; Test-ReviewFixtures passes (32 cases, 16 leaves).
This commit is contained in:
parent
1687b57c99
commit
e00ef8ad94
9 changed files with 412 additions and 0 deletions
|
|
@ -0,0 +1,48 @@
|
||||||
|
table 50100 "Order Header"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "Entry No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "Entry No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// No OnDelete. Deleting a header silently orphans every Order Line that
|
||||||
|
// belonged to it. Nothing errors, and no page shows the stranded rows.
|
||||||
|
}
|
||||||
|
|
||||||
|
table 50101 "Order Line"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "Line No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
field(2; "Header Entry No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
// Reads like referential integrity. It is lookup and input validation
|
||||||
|
// only: it propagates a RENAME of the parent key, and cascades nothing
|
||||||
|
// on DELETE.
|
||||||
|
TableRelation = "Order Header"."Entry No.";
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "Line No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,55 @@
|
||||||
|
table 50100 "Order Header"
|
||||||
|
{
|
||||||
|
// The owning table needs delete rights on what it owns. Granting D only on the
|
||||||
|
// header is a common miss and makes OnDelete fail for a non-SUPER user.
|
||||||
|
Permissions = tabledata "Order Line" = rd;
|
||||||
|
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "Entry No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "Entry No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
trigger OnDelete()
|
||||||
|
var
|
||||||
|
OrderLine: Record "Order Line";
|
||||||
|
begin
|
||||||
|
OrderLine.SetRange("Header Entry No.", "Entry No.");
|
||||||
|
// Pass false only when Order Line has no OnDelete of its own.
|
||||||
|
OrderLine.DeleteAll(true);
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
||||||
|
table 50101 "Order Line"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "Line No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
field(2; "Header Entry No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
TableRelation = "Order Header"."Entry No.";
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "Line No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,36 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: data-modeling
|
||||||
|
keywords: [ondelete, cascade, table-relation, orphan-records, header-line, dependent-records, referential-integrity]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# A table that owns dependent records must delete them in `OnDelete`
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`TableRelation` looks like referential integrity but only performs lookup and input validation. AL has **no cascading delete**: deleting a parent record leaves every dependent row untouched, and no error is raised.
|
||||||
|
|
||||||
|
What makes this specifically missable is an asymmetry. The platform *does* keep references correct on **rename** — renaming a record updates it in all other locations that declare a `TableRelation` to it, with no code. Delete has no equivalent. Same relation, same metadata, opposite behaviour. A developer who correctly learns that `TableRelation` "keeps references consistent" from the rename case, and generalises it to delete, ships orphans.
|
||||||
|
|
||||||
|
Orphaned rows are usually invisible, because a dependent table rarely has a page of its own. They inflate the table, break later reconciliation, and are re-encountered by duplicate checks when the parent key is reused.
|
||||||
|
|
||||||
|
This applies to internal, staging and `SystemMetadata` tables too. A table having no delete action in the UI today is not protection: a permission set that grants `D` on the table is evidence that deletion is anticipated.
|
||||||
|
|
||||||
|
See also `validate-table-relation-false-suppresses-rename-propagation.md` for the two preconditions on the rename half of this asymmetry.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
The owning table implements `OnDelete` and deletes its dependents there, filtered on the foreign key. Declare `Permissions = tabledata <dependent> = rd` on the owning table — granting delete rights only on the parent is a common miss that makes the trigger fail for a non-`SUPER` user. This mirrors the base application, where every header table deletes its own lines.
|
||||||
|
|
||||||
|
See sample: `owning-table-must-delete-dependents-in-ondelete.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
A parent table with dependent rows and no `OnDelete` trigger, where the dependent's foreign-key field declares a `TableRelation` back to the parent. The relation reads as if it guarantees integrity; it does not.
|
||||||
|
|
||||||
|
Detection signal: a table declares `TableRelation` to table X, and table X has no `OnDelete` trigger. Whether a delete path currently exists in the UI is irrelevant to the finding.
|
||||||
|
|
||||||
|
See sample: `owning-table-must-delete-dependents-in-ondelete.bad.al`.
|
||||||
|
|
@ -0,0 +1,35 @@
|
||||||
|
table 50121 "Document Link"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "Entry No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
field(2; "Document No."; Code[20])
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
TableRelation = "Source Document"."No.";
|
||||||
|
// Added to silence a validation error while the row is staged, before
|
||||||
|
// the Source Document exists. The relation is still declared, so this
|
||||||
|
// reads as harmless — but it also switches OFF rename propagation.
|
||||||
|
// Renaming a Source Document now leaves this field on the old key,
|
||||||
|
// with no error, and nothing else maintains it.
|
||||||
|
ValidateTableRelation = false;
|
||||||
|
}
|
||||||
|
// Composite value: no TableRelation is possible, and no OnRename on the
|
||||||
|
// owning table maintains it either. Rots the same way, for the other reason.
|
||||||
|
field(3; "Source Key"; Code[50])
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "Entry No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,70 @@
|
||||||
|
table 50120 "Source Document"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "No."; Code[20])
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// "Source Key" below cannot declare a TableRelation, so the platform cannot
|
||||||
|
// repoint it. The owning table carries the relationship by hand.
|
||||||
|
trigger OnRename()
|
||||||
|
var
|
||||||
|
DocumentLink: Record "Document Link";
|
||||||
|
begin
|
||||||
|
// In OnRename, xRec holds the PREVIOUS primary key while Rec holds the new
|
||||||
|
// one — the one trigger where that is true regardless of what drove the rename.
|
||||||
|
DocumentLink.SetRange("Source Key", MakeSourceKey(xRec."No."));
|
||||||
|
if DocumentLink.FindSet(true) then
|
||||||
|
repeat
|
||||||
|
DocumentLink."Source Key" := MakeSourceKey(Rec."No.");
|
||||||
|
DocumentLink.Modify(true);
|
||||||
|
until DocumentLink.Next() = 0;
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure MakeSourceKey(DocumentNo: Code[20]): Code[50]
|
||||||
|
begin
|
||||||
|
exit(StrSubstNo('DOC|%1', DocumentNo));
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
||||||
|
table 50121 "Document Link"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "Entry No."; Integer)
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
// Default ValidateTableRelation: the platform repoints this on rename.
|
||||||
|
field(2; "Document No."; Code[20])
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
TableRelation = "Source Document"."No.";
|
||||||
|
}
|
||||||
|
// Composite value — no TableRelation can express it, so the parent's
|
||||||
|
// OnRename above maintains it explicitly.
|
||||||
|
field(3; "Source Key"; Code[50])
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "Entry No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,38 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: data-modeling
|
||||||
|
keywords: [validatetablerelation, table-relation, rename, onrename, propagation, dangling-reference, soft-relation]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# `ValidateTableRelation = false` suppresses rename propagation, not just input validation
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
Renaming a record updates it in all other locations that reference it through a `TableRelation`, with no code. That guarantee has **two** preconditions, and a field failing either one is silently left holding a key that no longer exists.
|
||||||
|
|
||||||
|
First, a `TableRelation` must exist. A field whose value is constructed or computed — a composite key, or a value derived from several fields of the target — cannot declare one, so nothing propagates. The field is still a foreign key in intent, but the platform treats it as an opaque value.
|
||||||
|
|
||||||
|
Second, and far less obvious: the relation must not carry `ValidateTableRelation = false`. The property name implies it only governs *input validation*, so it looks safe to disable on a field populated by code that already knows the target is valid. It is not. **Disabling it also switches off rename propagation.** The relation still documents intent and still drives lookups, but it no longer keeps the stored value correct.
|
||||||
|
|
||||||
|
Both failures are quiet: no error at rename time, and in the first case no input validation either, so a wrong value is never rejected on write.
|
||||||
|
|
||||||
|
This is verified behaviour, not inference. A parent renamed once against a child holding three fields — a normal relation, the same relation with `ValidateTableRelation = false`, and a field with no relation — updates only the first.
|
||||||
|
|
||||||
|
See also `owning-table-must-delete-dependents-in-ondelete.md` for the delete half of this asymmetry, and `xrec-is-a-before-image-only-in-some-triggers.md` for why `OnRename` is the one trigger where a hand-written fix-up is reliable.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Leave `ValidateTableRelation` at its default wherever the stored value must stay correct across a rename. When it must be disabled, or when the relationship cannot be expressed as a `TableRelation` at all, the table owning the referenced key carries an explicit `OnRename` that repoints the dependents itself.
|
||||||
|
|
||||||
|
See sample: `validate-table-relation-false-suppresses-rename-propagation.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
`ValidateTableRelation = false` added to silence a validation error, on a field expected to keep tracking its target. The field stops being maintained on rename, and the defect surfaces much later as a reference to a key that no longer exists.
|
||||||
|
|
||||||
|
Detection signal: any `ValidateTableRelation = false` on a field that also declares a `TableRelation`. Ask what repoints the value when the target is renamed; if the answer is "the platform", the finding stands.
|
||||||
|
|
||||||
|
See sample: `validate-table-relation-false-suppresses-rename-propagation.bad.al`.
|
||||||
|
|
@ -0,0 +1,36 @@
|
||||||
|
table 50130 "Service Request"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "No."; Code[20])
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
field(2; Status; Enum "Service Request Status")
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
trigger OnModify()
|
||||||
|
begin
|
||||||
|
// Dead branch under any code-driven Modify: from code xRec mirrors Rec, so
|
||||||
|
// the two Status values are always equal and LogStatusChange never runs.
|
||||||
|
// Editing the field on a page DOES populate xRec, so this passes manual
|
||||||
|
// testing and then silently does nothing in a job queue or API call.
|
||||||
|
if Status <> xRec.Status then
|
||||||
|
LogStatusChange(xRec.Status, Status);
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure LogStatusChange(FromStatus: Enum "Service Request Status"; ToStatus: Enum "Service Request Status")
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,58 @@
|
||||||
|
table 50130 "Service Request"
|
||||||
|
{
|
||||||
|
fields
|
||||||
|
{
|
||||||
|
field(1; "No."; Code[20])
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
field(2; Status; Enum "Service Request Status")
|
||||||
|
{
|
||||||
|
DataClassification = CustomerContent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
keys
|
||||||
|
{
|
||||||
|
key(PK; "No.")
|
||||||
|
{
|
||||||
|
Clustered = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// OnModify: xRec mirrors Rec when the write came from code, so it cannot be
|
||||||
|
// used as a before-image. Re-read the stored row instead — this behaves the
|
||||||
|
// same whether a page, a job queue or an API drove the write.
|
||||||
|
trigger OnModify()
|
||||||
|
var
|
||||||
|
Previous: Record "Service Request";
|
||||||
|
begin
|
||||||
|
if Previous.Get("No.") and (Previous.Status <> Status) then
|
||||||
|
LogStatusChange(Previous.Status, Status);
|
||||||
|
end;
|
||||||
|
|
||||||
|
// OnRename: xRec IS the before-image of the primary key here, whatever drove
|
||||||
|
// the rename. This is the one trigger where the idiom is reliable.
|
||||||
|
trigger OnRename()
|
||||||
|
begin
|
||||||
|
RepointDependents(xRec."No.", "No.");
|
||||||
|
end;
|
||||||
|
|
||||||
|
// OnDelete: xRec reflects the record being removed.
|
||||||
|
trigger OnDelete()
|
||||||
|
begin
|
||||||
|
ArchiveRequest(xRec."No.");
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure LogStatusChange(FromStatus: Enum "Service Request Status"; ToStatus: Enum "Service Request Status")
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure RepointDependents(OldNo: Code[20]; NewNo: Code[20])
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
|
||||||
|
local procedure ArchiveRequest(RequestNo: Code[20])
|
||||||
|
begin
|
||||||
|
end;
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,36 @@
|
||||||
|
---
|
||||||
|
bc-version: [all]
|
||||||
|
domain: data-modeling
|
||||||
|
keywords: [xrec, before-image, onmodify, onrename, oninsert, ondelete, table-trigger, page-driven]
|
||||||
|
technologies: [al]
|
||||||
|
countries: [w1]
|
||||||
|
application-area: [all]
|
||||||
|
---
|
||||||
|
|
||||||
|
# `xRec` is a before-image in `OnRename` and `OnDelete`, but mirrors `Rec` in `OnInsert` and `OnModify` from code
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
`xRec` is widely believed to be "the previous record" inside every table trigger, and — in reaction to that — is often dismissed with the folk rule *"`xRec` only works from a page, never from code"*. Both are wrong, and the second is wrong in the place it matters most.
|
||||||
|
|
||||||
|
The behaviour is per-trigger. In `OnRename` and `OnDelete`, `xRec` is a genuine before-image regardless of what drove the write. In `OnInsert` and `OnModify`, a **code-driven** write leaves `xRec` mirroring `Rec` — there is no before-image at all — while a **page-driven** write does supply one.
|
||||||
|
|
||||||
|
That combination produces a defect that is unusually hard to catch. A comparison such as `if Rec.Status <> xRec.Status then` inside `OnModify` works when a tester clicks through a page, and silently never fires when the same code path runs from a job queue, a batch routine, or an API call. It fails as a no-op, not as an error.
|
||||||
|
|
||||||
|
The `OnRename` case is the useful half: because `xRec` there holds the previous primary key even from code, it is the one place a hand-written key fix-up is reliable. Note that in `OnRename` only the key differs between `Rec` and `xRec` — non-key field values are identical on both sides.
|
||||||
|
|
||||||
|
See also `validate-table-relation-false-suppresses-rename-propagation.md`, which describes when such a hand-written `OnRename` fix-up is required.
|
||||||
|
|
||||||
|
## Best Practice
|
||||||
|
|
||||||
|
Use `xRec` for the previous key in `OnRename`, and for the record being removed in `OnDelete`. In `OnModify`, obtain the before-image by re-reading the stored row rather than trusting `xRec`, so the logic behaves identically whether a page, a job queue or an API drove the write.
|
||||||
|
|
||||||
|
See sample: `xrec-is-a-before-image-only-in-some-triggers.good.al`.
|
||||||
|
|
||||||
|
## Anti Pattern
|
||||||
|
|
||||||
|
Comparing `Rec` against `xRec` inside `OnModify` (or `OnInsert`) to detect a change. From code the two are equal, so the branch is dead and whatever it guards never happens.
|
||||||
|
|
||||||
|
Detection signal: any read of `xRec` inside `OnModify` or `OnInsert`. Treat "but it works when I test it on the page" as confirmation of the defect rather than a refutation.
|
||||||
|
|
||||||
|
See sample: `xrec-is-a-before-image-only-in-some-triggers.bad.al`.
|
||||||
Loading…
Add table
Add a link
Reference in a new issue