mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Add breaking-changes knowledge domain and review leaf skill (#44)
Seed a new breaking-changes (AL API stability) knowledge domain with six articles plus good/bad AL samples, a new al-breaking-changes-review leaf skill, and minimal wiring into al-code-review and the README. Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
f19f0618fb
commit
6140a52b03
21 changed files with 503 additions and 3 deletions
|
|
@ -0,0 +1,21 @@
|
|||
codeunit 50326 "Order Processor Bad"
|
||||
{
|
||||
// Anti-pattern: every helper is public by default, exposing implementation
|
||||
// detail as a de-facto API. Each becomes a contract that cannot be changed
|
||||
// without risking breakage for consumers that bound to it.
|
||||
procedure ProcessOrder(OrderNo: Code[20])
|
||||
begin
|
||||
ValidateOrder(OrderNo);
|
||||
PostOrder(OrderNo);
|
||||
end;
|
||||
|
||||
procedure ValidateOrder(OrderNo: Code[20])
|
||||
begin
|
||||
if OrderNo = '' then
|
||||
Error('Order number is required.');
|
||||
end;
|
||||
|
||||
procedure PostOrder(OrderNo: Code[20])
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,21 @@
|
|||
codeunit 50325 "Order Processor Good"
|
||||
{
|
||||
// Supported, stable entry point — intentionally public.
|
||||
procedure ProcessOrder(OrderNo: Code[20])
|
||||
begin
|
||||
ValidateOrder(OrderNo);
|
||||
PostOrder(OrderNo);
|
||||
end;
|
||||
|
||||
// In-app reuse only — internal, so it is not part of the external contract.
|
||||
internal procedure ValidateOrder(OrderNo: Code[20])
|
||||
begin
|
||||
if OrderNo = '' then
|
||||
Error('Order number is required.');
|
||||
end;
|
||||
|
||||
// Implementation detail confined to this object — local.
|
||||
local procedure PostOrder(OrderNo: Code[20])
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: breaking-changes
|
||||
keywords: [access-modifier, internal, local, public, protected, scope, encapsulation]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Choose access modifiers deliberately
|
||||
|
||||
## Description
|
||||
|
||||
Access is a decision about what you are willing to support forever. The moment a procedure or object is reachable from another extension — no `local`, or a removed `[Scope('OnPrem')]` — it becomes a contract: callers bind to it, and changing or removing it later is a breaking change. The safe default is the narrowest access that works. Use `local` for implementation detail confined to one object, `internal` for code shared within the app but not exposed to consumers, and `protected var` for state intended as an inheritance point for extension objects. Reserve `public` for the deliberate, supported entry points you intend to maintain as a stable API. LLMs tend to make everything public "to be safe," which inverts the rule and turns every helper into an accidental contract.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Start everything `local` or `internal` and promote a member to `public` only when you have decided to support it as a stable contract. Expose a small, intentional surface — the supported entry point — and keep validation, posting, and helper routines `internal` for in-app reuse or `local` when single-object. Do not drop `[Scope('OnPrem')]` without intent, since that too widens the contract. Every public member is a maintenance commitment; spend them deliberately.
|
||||
|
||||
See sample: `choose-access-modifiers-deliberately.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Declaring every procedure `public` by default, so internal helpers like `ValidateOrder` and `PostOrder` become a de-facto API that consumers bind to and that can no longer be changed freely. Detection: an object where implementation-detail procedures carry no access modifier or are `public` without a reason to support them externally. Default them to `internal`/`local` and make only the intended entry point public.
|
||||
|
||||
See sample: `choose-access-modifiers-deliberately.bad.al`.
|
||||
|
|
@ -0,0 +1,10 @@
|
|||
codeunit 50306 "Net Amount Api Bad"
|
||||
{
|
||||
// Breaking: the published CalcNet procedure was renamed outright with no
|
||||
// deprecation window and no [Obsolete] marker. Every extension that called
|
||||
// CalcNet breaks the instant it consumes this version.
|
||||
procedure CalculateNetAmount(GrossAmount: Decimal; TaxRate: Decimal): Decimal
|
||||
begin
|
||||
exit(GrossAmount / (1 + TaxRate));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 50305 "Net Amount Api Good"
|
||||
{
|
||||
// Old name kept and marked obsolete: callers still compile but get a warning
|
||||
// pointing at the replacement, with a tag recording the removal target version.
|
||||
[Obsolete('Use CalculateNetAmount instead.', '25.0')]
|
||||
procedure CalcNet(GrossAmount: Decimal; TaxRate: Decimal): Decimal
|
||||
begin
|
||||
exit(CalculateNetAmount(GrossAmount, TaxRate));
|
||||
end;
|
||||
|
||||
procedure CalculateNetAmount(GrossAmount: Decimal; TaxRate: Decimal): Decimal
|
||||
begin
|
||||
exit(GrossAmount / (1 + TaxRate));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: breaking-changes
|
||||
keywords: [obsolete, deprecation, obsoletestate, obsoletetag, pending, removed, public-procedure]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Deprecate public members through the Obsolete lifecycle, never delete them outright
|
||||
|
||||
## Description
|
||||
|
||||
Deleting or renaming a published procedure (or object) in a single release is a hard break: dependent extensions that reference it stop compiling the moment they pick up the new version, with no warning window to migrate. AL provides a staged deprecation lifecycle precisely so consumers get advance notice. For a procedure, apply the `[Obsolete('reason', 'tag')]` attribute: the member keeps working but every caller gets a compiler warning naming the replacement and the target version. The member stays through a deprecation window — at least one major release — before it is finally removed. Object- and field-level members use the matching `ObsoleteState = Pending` → `Removed` property progression. LLMs trained to "clean up" code often delete or rename the old member immediately, skipping the window entirely.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When a published procedure is superseded, keep it in place and mark it `[Obsolete('Use CalculateNetAmount instead.', '25.0')]`, where the message names the replacement and the tag records the target version for removal. Have the obsolete member forward to the new one so behavior is preserved during the window. Only after the deprecation window has elapsed — a later release — change its state to removed. This gives every dependent app a compile-time signal and time to migrate before anything actually disappears.
|
||||
|
||||
See sample: `deprecate-public-members-with-the-obsolete-lifecycle.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Renaming or deleting the published `CalcNet` procedure in place — replacing it with `CalculateNetAmount` and nothing else — so consumers calling `CalcNet` break immediately with no deprecation notice. Detection: a previously shipped non-`local` procedure that vanished or was renamed between versions with no `[Obsolete]` marker left behind on a kept member. Mark it obsolete and keep it for a window instead.
|
||||
|
||||
See sample: `deprecate-public-members-with-the-obsolete-lifecycle.bad.al`.
|
||||
|
|
@ -0,0 +1,10 @@
|
|||
codeunit 50301 "Discount Api Bad"
|
||||
{
|
||||
// Breaking: a Rate parameter was added to a procedure that already shipped.
|
||||
// Every dependent extension that called CalculateDiscount(Amount) now fails
|
||||
// to compile until it is changed and recompiled.
|
||||
procedure CalculateDiscount(Amount: Decimal; Rate: Decimal): Decimal
|
||||
begin
|
||||
exit(Amount * Rate);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 50300 "Discount Api Good"
|
||||
{
|
||||
// Published contract — signature kept exactly as it shipped.
|
||||
procedure CalculateDiscount(Amount: Decimal): Decimal
|
||||
begin
|
||||
exit(Amount * 0.05);
|
||||
end;
|
||||
|
||||
// New capability added as a separate overload, so existing callers of
|
||||
// CalculateDiscount(Amount) keep compiling. The return value is named, which
|
||||
// is the one signature change that is always safe to make.
|
||||
procedure CalculateDiscountWithRate(Amount: Decimal; Rate: Decimal) Discount: Decimal
|
||||
begin
|
||||
Discount := Amount * Rate;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: breaking-changes
|
||||
keywords: [signature, public-procedure, parameter, return-value, overload, contract]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not change the signature of a published procedure
|
||||
|
||||
## Description
|
||||
|
||||
A procedure that is reachable from outside its object — any procedure not marked `local` (and, for on-prem-scoped code, anything a dependent app can still bind to) — is a contract. Once another extension compiles against it, changing its shape breaks that extension at build time. Signature changes include adding, removing, or reordering parameters, changing a parameter or return type, and toggling a parameter between by-value and `var` (by-reference). The platform treats the procedure's identity as its full signature, so even a "compatible-looking" tweak is a new method to dependents. There is exactly one safe edit: naming a previously unnamed return value, which adds no caller obligation. LLMs routinely "improve" a public procedure in place by adding a parameter, not realizing every consumer must be recompiled.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Treat a published signature as frozen. When new behavior needs more inputs, add a new procedure or overload alongside the original — for example a `CalculateDiscountWithRate(Amount; Rate)` next to the unchanged `CalculateDiscount(Amount)` — and let the old one delegate to the new one. Existing callers keep compiling; new callers opt into the richer entry point. Naming an unnamed return value is the one in-place change that is always safe.
|
||||
|
||||
See sample: `do-not-change-published-procedure-signatures.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Editing the existing public procedure's parameter list — here, adding a `Rate` parameter to `CalculateDiscount` — so every dependent extension that called the old form fails to compile. Detection: a parameter added, removed, reordered, retyped, or flipped to/from `var`, or a changed return type, on any non-`local` procedure that already shipped. Add a new overload instead.
|
||||
|
||||
See sample: `do-not-change-published-procedure-signatures.bad.al`.
|
||||
|
|
@ -0,0 +1,13 @@
|
|||
codeunit 50321 "Payment Client Bad"
|
||||
{
|
||||
var
|
||||
AccessToken: Text;
|
||||
|
||||
// Crossing the trust boundary: a public getter hands the raw credential to any
|
||||
// caller, turning a secret into a de-facto public API that cannot be removed
|
||||
// later without breaking consumers.
|
||||
procedure GetAccessToken(): Text
|
||||
begin
|
||||
exit(AccessToken);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,25 @@
|
|||
codeunit 50320 "Payment Client Good"
|
||||
{
|
||||
var
|
||||
AccessToken: Text;
|
||||
|
||||
// Credential flows inward through an internal setter and never leaves the object.
|
||||
internal procedure SetAccessToken(NewToken: Text)
|
||||
begin
|
||||
AccessToken := NewToken;
|
||||
end;
|
||||
|
||||
// Public API exposes only non-sensitive data — a masked reference, never the token.
|
||||
procedure GetMaskedReference(): Text
|
||||
var
|
||||
Reference: Text;
|
||||
begin
|
||||
Reference := LastReference();
|
||||
exit('****-' + CopyStr(Reference, StrLen(Reference) - 3));
|
||||
end;
|
||||
|
||||
local procedure LastReference(): Text
|
||||
begin
|
||||
exit('REF000123456');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: breaking-changes
|
||||
keywords: [sensitive-data, secrettext, token, credential, public-api, access-boundary]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not widen access to expose sensitive data through a public API
|
||||
|
||||
## Description
|
||||
|
||||
Every member you make publicly reachable becomes a contract you must keep — and when that member returns a secret, the contract leaks the secret. Widening access to a credential happens in several shapes: a public getter that returns a raw token or password, an event whose parameter carries a secret to every subscriber, or a global variable holding a key that an extension can read. Once such a surface ships, removing it is itself a breaking change, so the exposure is hard to walk back. Sensitive material — tokens, passwords, connection secrets, `SecretText` values, security internals — must stay inside `internal` or `local` members. Public surfaces should expose only non-sensitive business data. LLMs often add a convenient `GetToken()` getter without recognizing it as a permanent security boundary breach.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Keep secrets in `internal` or `local` members, and prefer the `SecretText` type so the value cannot be read back or logged. Where callers genuinely need a credential, pass it inward (a setter) rather than handing it outward (a getter). Public API should return only non-sensitive data — a masked reference, a status, a business identifier — never the raw secret. Treat each public member as a lasting commitment and keep the security-sensitive surface as small as possible.
|
||||
|
||||
See sample: `do-not-expose-sensitive-data-through-public-api.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A public `GetAccessToken()` that returns the raw token (or an event parameter carrying a credential to all subscribers), turning a secret into a de-facto public API any dependent can consume. Detection: a non-`local` procedure, event parameter, or global variable that surfaces a token, password, key, or other credential. Keep the secret internal and expose only non-sensitive data.
|
||||
|
||||
See sample: `do-not-expose-sensitive-data-through-public-api.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
codeunit 50316 "Pricing Api Bad"
|
||||
{
|
||||
// Anti-pattern: new surcharge logic is added inside a procedure already marked
|
||||
// obsolete, and inside a #if not CLEAN25 block. Both are scheduled for removal,
|
||||
// so this behaviour disappears the moment CLEAN25 is enabled.
|
||||
[Obsolete('Use GetUnitPrice instead.', '25.0')]
|
||||
procedure GetPrice(ItemNo: Code[20]): Decimal
|
||||
var
|
||||
Price: Decimal;
|
||||
begin
|
||||
Price := 100;
|
||||
#if not CLEAN25
|
||||
Price += CalculateSurcharge(ItemNo);
|
||||
#endif
|
||||
exit(Price);
|
||||
end;
|
||||
|
||||
local procedure CalculateSurcharge(ItemNo: Code[20]): Decimal
|
||||
begin
|
||||
exit(5);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
codeunit 50315 "Pricing Api Good"
|
||||
{
|
||||
// Obsolete member left untouched — it only forwards to the replacement and
|
||||
// gains no new logic.
|
||||
[Obsolete('Use GetUnitPrice instead.', '25.0')]
|
||||
procedure GetPrice(ItemNo: Code[20]): Decimal
|
||||
begin
|
||||
exit(GetUnitPrice(ItemNo));
|
||||
end;
|
||||
|
||||
// New behaviour is built on the supported replacement, not on the obsolete member.
|
||||
procedure GetUnitPrice(ItemNo: Code[20]): Decimal
|
||||
begin
|
||||
exit(CalculateBasePrice(ItemNo) + CalculateSurcharge(ItemNo));
|
||||
end;
|
||||
|
||||
local procedure CalculateBasePrice(ItemNo: Code[20]): Decimal
|
||||
begin
|
||||
exit(100);
|
||||
end;
|
||||
|
||||
local procedure CalculateSurcharge(ItemNo: Code[20]): Decimal
|
||||
begin
|
||||
exit(5);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: breaking-changes
|
||||
keywords: [obsolete, clean-flag, conditional-compilation, deprecation, replacement, do-not-extend]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not build on code already marked obsolete
|
||||
|
||||
## Description
|
||||
|
||||
A member carrying `[Obsolete]`, or wrapped in a `#if not CLEANxx` conditional-compilation block, is already scheduled for deletion — the `CLEANxx` symbol is flipped on in a future release to strip that code out. Adding logic, raising new events, or taking fresh dependencies on such a member ties live behavior to something the platform is about to remove. When the deprecation completes, everything layered on top breaks. The obsolete marker is a one-way signal: it means "migrate off," never "safe to extend." LLMs frequently edit whatever procedure is nearest to the change, including obsolete ones, and add `#if not CLEANxx` branches without understanding that the block is transient.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Leave obsolete members exactly as they are and implement against the current, supported replacement. New logic — a surcharge calculation, an event publisher, a hook — belongs on the live API (`GetUnitPrice`), never inside the deprecated `GetPrice` or behind a `#if not CLEAN25` guard. If the replacement does not yet exist, create it as a first-class member and build there. The obsolete code should only shrink over time, not accrete new behavior.
|
||||
|
||||
See sample: `do-not-modify-code-already-marked-obsolete.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding a surcharge calculation inside the `[Obsolete]` `GetPrice` procedure, or behind a `#if not CLEAN25` block, so the new behavior is wired to code that will be removed when `CLEAN25` is enabled. Detection: new statements, event declarations, or dependencies introduced inside an `[Obsolete]`-marked member or a `#if not CLEANxx` region. Move the logic onto the supported replacement instead.
|
||||
|
||||
See sample: `do-not-modify-code-already-marked-obsolete.bad.al`.
|
||||
|
|
@ -0,0 +1,11 @@
|
|||
table 50311 "Customer Profile Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
// Breaking: the published "Email" field was renamed in place. Dependent
|
||||
// extensions that reference "Email" stop compiling, and the data stored in
|
||||
// the old column is orphaned on upgrade.
|
||||
field(2; "Contact Email"; Text[80]) { }
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,17 @@
|
|||
table 50310 "Customer Profile Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
// Replacement field shipped alongside the old one.
|
||||
field(2; "Contact Email"; Text[80]) { }
|
||||
// Old field kept and marked Pending so dependent code keeps compiling and
|
||||
// an upgrade codeunit can copy its data before it is finally removed.
|
||||
field(3; "Email"; Text[80])
|
||||
{
|
||||
ObsoleteState = Pending;
|
||||
ObsoleteReason = 'Replaced by Contact Email. Will be removed after the deprecation window.';
|
||||
ObsoleteTag = '25.0';
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: breaking-changes
|
||||
keywords: [table-field, obsoletestate, obsoletereason, obsoletetag, pending, removed, data-loss]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Obsolete published table fields instead of deleting or renaming them
|
||||
|
||||
## Description
|
||||
|
||||
A table field that has shipped carries two contracts at once: extensions reference it by name, and the database holds data in its column. Deleting the field, or renaming it (which the platform treats as drop-plus-add), breaks dependent code at compile time and discards the stored data — a silent data-loss event on upgrade. The fix is the same staged lifecycle used for objects: set `ObsoleteState = Pending` together with `ObsoleteReason` and an `ObsoleteTag` naming the target version, ship the new field alongside, migrate data during the window, and only switch the old field to `ObsoleteState = Removed` in a later release once nothing depends on it. LLMs often "tidy" a schema by renaming a field in place, not realizing this is both a breaking change and a data-loss risk.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Add the replacement field, then mark the old field `ObsoleteState = Pending` with an `ObsoleteReason` that names the replacement and an `ObsoleteTag` carrying the target version (for example `'25.0'`). Keep the obsolete field readable so an upgrade codeunit can copy its data into the new field during the deprecation window. Move it to `ObsoleteState = Removed` only in a later major version, after the window has passed and data has migrated.
|
||||
|
||||
See sample: `obsolete-table-fields-instead-of-deleting-them.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Renaming the published `Email` field to `Contact Email` directly in the table — or deleting it — so dependent extensions that reference `Email` break and the column's stored values are orphaned on upgrade. Detection: a previously shipped field removed or renamed in a table or table extension with no `ObsoleteState = Pending` step preserving the original. Obsolete the field through the lifecycle instead.
|
||||
|
||||
See sample: `obsolete-table-fields-instead-of-deleting-them.bad.al`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue