mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
Promote knowledge for Microsoft review skills (#153)
Move canonical knowledge for Microsoft-owned review domains into the Microsoft layer and document the skill/knowledge co-location policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Copilot-Session: 2a6ea875-d38e-4f30-aadb-0d606f9be231
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
bca8f478d8
commit
4f0a13a801
88 changed files with 11 additions and 7 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,55 @@
|
|||
table 50123 "Transfer Source Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
field(2; "Reference"; Code[20])
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.")
|
||||
{
|
||||
Clustered = true;
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
table 50124 "Transfer Target Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
field(2; "Reference"; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.")
|
||||
{
|
||||
Clustered = true;
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
codeunit 50492 "TransferFields Bad"
|
||||
{
|
||||
procedure CopyData(Source: Record "Transfer Source Bad"; var Target: Record "Transfer Target Bad")
|
||||
begin
|
||||
Target.TransferFields(Source, true, true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,59 @@
|
|||
table 50121 "Transfer Source"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
field(2; "Reference"; Code[20])
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.")
|
||||
{
|
||||
Clustered = true;
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
table 50122 "Transfer Target"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
field(2; "Reference"; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.")
|
||||
{
|
||||
Clustered = true;
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
codeunit 50491 "TransferFields Good"
|
||||
{
|
||||
procedure CopyData(Source: Record "Transfer Source"; var Target: Record "Transfer Target")
|
||||
var
|
||||
ConvertedReference: Integer;
|
||||
begin
|
||||
Target."Entry No." := Source."Entry No.";
|
||||
Evaluate(ConvertedReference, Source."Reference");
|
||||
Target.Validate("Reference", ConvertedReference);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [16..]
|
||||
domain: data-modeling
|
||||
keywords: [transferfields, skipfieldsnotmatchingtype, type-mismatch, field-mapping, data-transfer]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not use SkipFieldsNotMatchingType to hide required TransferFields mismatches
|
||||
|
||||
## Description
|
||||
|
||||
`Record.TransferFields` copies values between fields with matching field numbers. Without `SkipFieldsNotMatchingType` (or with it `false`), a type mismatch between two fields in the same extension raises a runtime error at the point of transfer. Setting `SkipFieldsNotMatchingType` to `true` removes that error: the field is skipped instead, and the rest of the transfer completes normally. The caller gets no indication that a field was not copied.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `TransferFields(Source)` only when every field the destination requires, including primary key fields, is guaranteed to share a matching field number and type with the source; this form defaults `InitPrimaryKeyFields` to `true`. Fields with no matching field number, and fields whose types differ across extensions, are skipped regardless of `SkipFieldsNotMatchingType` — that parameter only governs same-extension type mismatches. If the destination depends on a field that falls into either case, map and validate it explicitly in code rather than relying on `TransferFields` to catch the gap. Use `SkipFieldsNotMatchingType = true` only when skipping same-extension type mismatches is an intentional, documented part of the transfer contract.
|
||||
|
||||
See sample: `transferfields-skip-type-mismatch-can-drop-data.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Using `TransferFields(Source, InitPrimaryKeyFields, true)` as a generic way to make two evolving table schemas transfer without errors, when the destination depends on every required source field being copied. A type change on either table can turn a previously transferred field into a silently skipped one without making the transfer itself fail.
|
||||
|
||||
See sample: `transferfields-skip-type-mismatch-can-drop-data.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