mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Add 18 more community AL/BC patterns across appsource, data-modeling, error-handling, security, style, testing, ui, upgrade, and web-services
Second contribution from CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Cross-checked against the current microsoft/knowledge corpus before opening; several originally-drafted candidates were dropped as duplicates of existing files.
This commit is contained in:
parent
07e324ddbc
commit
a4d85c3e9e
50 changed files with 1262 additions and 0 deletions
|
|
@ -0,0 +1,11 @@
|
|||
// Both fields guarded the same way, out of habit rather than analysis.
|
||||
if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then
|
||||
VATRegNo := SalesHeader."VAT Registration No."; // low blast radius - fine
|
||||
|
||||
// but the same pattern, unexamined, was also applied here:
|
||||
if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then
|
||||
VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group"
|
||||
else
|
||||
VATBusPostingGroup := '';
|
||||
// High blast radius: silently wrong VAT posting group reaches posting
|
||||
// with no error, no TestField, and no reviewer in the loop.
|
||||
|
|
@ -0,0 +1,10 @@
|
|||
// Low blast radius: guard, with an explicit chosen fallback.
|
||||
if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then
|
||||
VATRegNo := SalesHeader."VAT Registration No.";
|
||||
// Blank is an acceptable, deliberately-considered default here - the field
|
||||
// is informational and a reviewer sees it before the document ships.
|
||||
|
||||
// High blast radius: let it fail loud, because this feeds posted VAT.
|
||||
SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo);
|
||||
SalesHeader.TestField("VAT Bus. Posting Group");
|
||||
VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group";
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: error-handling
|
||||
keywords: [defensive-programming, offensive-programming, fail-fast, blast-radius, guarded-lookup]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Match defensive vs. offensive error handling to the blast radius of being wrong
|
||||
|
||||
## Description
|
||||
|
||||
Whether code should guard gracefully (defensive) or fail loudly (offensive/fail-fast) is not a matter of habit or a blanket house style — it depends on what happens downstream if the guarded condition is silently defaulted or skipped. Treating every missing value the same way, defensively or offensively, is itself the anti-pattern: uniform defensiveness hides the failures that matter most, while uniform fail-fast turns ordinary, expected absence into unnecessary crashes. Two fields can look structurally identical — both read from a related record, both potentially missing — and still deserve opposite treatment depending on what they feed.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Trace what a silently-defaulted or skipped value actually reaches before deciding how to guard it. If it reaches a posted ledger amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output, code offensively: let the lookup fail loud (`TestField`, an unguarded `Get()` expected to always succeed, or an explicit `Error`) so a human sees the problem before anything posts. If it is cosmetic, informational, or easily corrected after the fact (a display field, an optional UI enhancement, a report not yet run), code defensively — but the fallback must be an explicit, deliberately-chosen, named business value, never a blank or zero that is merely the datatype default. When genuinely unsure which category a field falls into, that is a question to resolve explicitly with whoever owns the requirement, not a coin flip.
|
||||
|
||||
See sample: `defensive-vs-offensive-code-must-match-blast-radius.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Guarding two fields the same way purely out of habit, without analyzing what each one feeds. A low-blast-radius field, such as a VAT registration number shown only on a printed document, and a high-blast-radius field, such as the VAT posting group that determines VAT actually applied to a posted transaction, are both wrapped in the same `if Header.Get(...) then ... else` pattern with a blank/zero fallback — leaving the posting-critical field free to post with a silently wrong value.
|
||||
|
||||
See sample: `defensive-vs-offensive-code-must-match-blast-radius.bad.al`.
|
||||
|
|
@ -0,0 +1,24 @@
|
|||
codeunit 50101 "Sample Web Service Caller"
|
||||
{
|
||||
procedure CallExternalService()
|
||||
var
|
||||
ErrorLogEntry: Record "Sample Error Log";
|
||||
begin
|
||||
// BUG: the log write happens inside the same transaction as the
|
||||
// risky call, using the same Record instance as the caller.
|
||||
if not TryCallService() then begin
|
||||
ErrorLogEntry.Init();
|
||||
ErrorLogEntry."Error Message" := CopyStr(GetLastErrorText(), 1, 250);
|
||||
ErrorLogEntry.Insert();
|
||||
Error(GetLastErrorText());
|
||||
// Error() above rolls back this transaction - including the
|
||||
// Insert() just made. The failure is never actually logged.
|
||||
end;
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryCallService()
|
||||
begin
|
||||
// ... external call that may fail ...
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
codeunit 50100 "Sample Error Log Writer"
|
||||
{
|
||||
// Started via Session.StartSession so its commit is independent of the
|
||||
// caller's transaction. Does one thing: insert the log entry, commit.
|
||||
trigger OnRun()
|
||||
var
|
||||
ErrorLogEntry: Record "Sample Error Log";
|
||||
begin
|
||||
ErrorLogEntry.Init();
|
||||
ErrorLogEntry."Call Duration (ms)" := CallDurationMs;
|
||||
ErrorLogEntry."Error Message" :=
|
||||
CopyStr(ErrorMessageText, 1, MaxStrLen(ErrorLogEntry."Error Message"));
|
||||
ErrorLogEntry.Insert(true);
|
||||
Commit();
|
||||
end;
|
||||
|
||||
procedure SetParameters(Duration: Integer; ErrorText: Text)
|
||||
begin
|
||||
CallDurationMs := Duration;
|
||||
ErrorMessageText := ErrorText;
|
||||
end;
|
||||
|
||||
var
|
||||
CallDurationMs: Integer;
|
||||
ErrorMessageText: Text;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: error-handling
|
||||
keywords: [logging, rollback, session, transaction, isolated-session, telemetry]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Log writes that must capture failures must survive transaction rollback
|
||||
|
||||
## Description
|
||||
|
||||
Inserting a log record inside the same transaction as the operation it logs looks correct until the operation errors: the transaction rolls back and takes the log entry with it. The result is a log that faithfully records every success and silently loses exactly the failures it exists to capture. This is a common blind spot in error/duration logging around web-service calls, background jobs, and other operations expected to fail sometimes.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`StartSession` on a codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. Capture duration and other telemetry values in the caller and pass them as parameters — the isolated session must not re-read state that a rollback may have erased. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically.
|
||||
|
||||
See sample: `log-writes-must-survive-rollback.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Inserting the error-log record in the same transaction as the risky operation, so a rollback deletes the very entry meant to explain the failure. Adding a stray `Commit` before the risky call is not a fix either — it breaks the caller's atomicity and can violate posting-routine rules. Swallowing the error to keep the log alive (running a codeunit without checking or re-raising its result) is equally wrong: the log must observe the failure, not suppress it.
|
||||
|
||||
See sample: `log-writes-must-survive-rollback.bad.al`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue