mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Fix lifecycle compatibility guidance (#93)
* Fix lifecycle compatibility guidance Correct high-confidence Business Central guidance and samples for upgrade tags, collectible errors, trigger semantics, obsoletion, events, interfaces, API contracts, and test transactions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e05a43e7-6448-4d67-9c73-798523f5d945 * Address guidance review findings Gate SecretText guidance to BC23 and clarify that the collectible-error sample intentionally emits a message-only blocking aggregate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e05a43e7-6448-4d67-9c73-798523f5d945 --------- Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
aca3986fd0
commit
5706959e4a
32 changed files with 201 additions and 104 deletions
|
|
@ -7,8 +7,8 @@ codeunit 50221 "Upgrade Existing Field"
|
|||
Customer: Record Customer;
|
||||
DT: DataTransfer;
|
||||
begin
|
||||
// "Credit Limit (LCY)" has OnValidate logic that recalculates risk fields
|
||||
// and notifies subscribers. DataTransfer skips both — derived data drifts.
|
||||
// DataTransfer skips the field's OnValidate logic and validation events,
|
||||
// plus the table OnModify trigger and row-based modification events.
|
||||
DT.SetTables(Database::Customer, Database::Customer);
|
||||
DT.AddConstantValue(50000, Customer.FieldNo("Credit Limit (LCY)"));
|
||||
DT.CopyFields();
|
||||
|
|
|
|||
|
|
@ -1,16 +1,17 @@
|
|||
codeunit 50220 "Upgrade New Field Init"
|
||||
codeunit 50220 "Upgrade Trigger Aware"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
||||
local procedure InitializeNewFlagOnMyTable()
|
||||
local procedure UpdateCustomerCreditLimit()
|
||||
var
|
||||
MyTable: Record "My Table";
|
||||
DT: DataTransfer;
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
// "New Flag" is added in the same change as this upgrade procedure.
|
||||
// No existing validation logic depends on it, so DataTransfer is safe.
|
||||
DT.SetTables(Database::"My Table", Database::"My Table");
|
||||
DT.AddConstantValue(true, MyTable.FieldNo("New Flag"));
|
||||
DT.CopyFields();
|
||||
if Customer.FindSet(true) then
|
||||
repeat
|
||||
// Validate runs the field OnValidate logic; Modify(true) separately
|
||||
// runs the table OnModify trigger and its row-based events.
|
||||
Customer.Validate("Credit Limit (LCY)", 50000);
|
||||
Customer.Modify(true);
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,18 +11,18 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
`DataTransfer` writes directly at the database layer. It does not invoke field `OnValidate` triggers, table `OnModify` triggers, or any `OnAfterModifyEvent` / `OnBeforeValidate...` event subscribers that a normal `Record.Modify(true)` would. This is precisely what makes it fast — and precisely what makes it a footgun when the field being updated has validation logic that other code relies on. The receiving code never gets the signal that a row changed, derived fields stay stale, audit hooks do not run.
|
||||
`DataTransfer` writes sets directly at the database layer, so row-based triggers and events do not run. For `CopyFields`, that includes the table `OnModify` trigger and `OnBeforeModifyEvent`/`OnAfterModifyEvent`; direct field assignment also does not call field `OnValidate` or its validation events. These are separate behaviors: `Record.Validate(Field, Value)` runs field validation, while `Record.Modify(true)` runs the table `OnModify` trigger. Calling `Modify(true)` does not retroactively validate assigned fields.
|
||||
|
||||
For *new fields and tables added in the same change* this is fine: nothing yet depends on the validation. For *pre-existing fields with validation logic*, `DataTransfer` quietly bypasses business logic that may be load-bearing for posting, calculation, or integration scenarios.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `DataTransfer` only when the field or table is new in the same change — initial population is the canonical safe case. When updating a pre-existing field that has validation logic, either use `Modify(true)` to honour the triggers, or, if `DataTransfer` is still required for performance reasons, leave a comment that explicitly states "validation triggers and event subscribers are intentionally not raised" and verify with the field's owner that this is safe.
|
||||
Use `DataTransfer` when set-based transfer is safe and row-level business logic is intentionally unnecessary — initial population of a new field is the canonical case. When an existing field's validation must run, loop through records and call `Validate(Field, Value)`; if the table's modify trigger must also run, follow with `Modify(true)`. If performance requires `DataTransfer`, document exactly which field-validation and row-modification triggers or subscribers are intentionally bypassed and verify that derived data remains correct.
|
||||
|
||||
See sample: `datatransfer-skips-triggers-and-subscribers.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Reaching for `DataTransfer` to update an existing field with non-trivial `OnValidate` logic, without a comment and without confirming that subscribers can be skipped. The upgrade succeeds; runtime behaviour drifts silently.
|
||||
Reaching for `DataTransfer` to update an existing field with non-trivial `OnValidate` or `OnModify` logic, without confirming that both validation and row-modification subscribers can be skipped. Replacing it with only `Modify(true)` is also incomplete when field validation is required; call `Validate` for that field first.
|
||||
|
||||
See sample: `datatransfer-skips-triggers-and-subscribers.bad.al`.
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
codeunit 50228 "Old Method Holder"
|
||||
{
|
||||
// ObsoleteState set without ObsoleteReason or ObsoleteTag.
|
||||
[Obsolete('')]
|
||||
// Methods use the attribute, but empty reason and tag give no migration path.
|
||||
[Obsolete('', '')]
|
||||
procedure OldMethod()
|
||||
begin
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ codeunit 50227 "Old Method Holder"
|
|||
[Obsolete('Use NewMethod instead for better performance', '22.0')]
|
||||
procedure OldMethod()
|
||||
begin
|
||||
// Body kept while ObsoleteState = Pending; warns at call sites.
|
||||
// The method remains callable during its deprecation window.
|
||||
end;
|
||||
|
||||
procedure NewMethod()
|
||||
|
|
|
|||
|
|
@ -7,27 +7,26 @@ countries: [w1]
|
|||
application-area: [all]
|
||||
---
|
||||
|
||||
# Mark obsolete elements with `ObsoleteState`, `ObsoleteReason`, and `ObsoleteTag`
|
||||
# Give every obsolete element a reason and tag
|
||||
|
||||
## Description
|
||||
|
||||
When a procedure, field, table, page, or enum value is being retired, AL requires three pieces of metadata to declare the deprecation:
|
||||
AL has two obsoletion mechanisms, depending on the symbol:
|
||||
|
||||
- `ObsoleteState` — `Pending` while the element still exists but is being phased out, `Removed` once it should no longer be used.
|
||||
- `ObsoleteReason` — a short human-readable string explaining what to use instead. Tooling and downstream consumers surface this when warning callers.
|
||||
- `ObsoleteTag` — a stable version-like marker (typically the release version in which the deprecation was introduced, e.g. `'22.0'`).
|
||||
- Objects, fields, enum types, and enum values use the `ObsoleteState`, `ObsoleteReason`, and `ObsoleteTag` properties. `Pending` warns while the element remains available; `Removed` blocks references.
|
||||
- Methods, variables, events, and other symbols use `[Obsolete('reason', 'tag')]`. They do not have an `ObsoleteState` property.
|
||||
|
||||
Omitting `ObsoleteReason` or `ObsoleteTag` leaves consumers with `ObsoleteState = Pending` but no guidance and no traceability. Declaring `ObsoleteState = Removed` without a reason or tag is the same failure with a stronger blast radius.
|
||||
In both forms, the reason should name the replacement and the tag should identify when the element became obsolete. Empty or missing guidance leaves consumers without an actionable migration path.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Every obsoleted element carries all three properties together. The reason names the replacement explicitly; the tag is the version in which the deprecation was introduced and stays stable for the life of the deprecation.
|
||||
For an object or field, set all three properties together. For a method, variable, or event, provide both `[Obsolete]` arguments. Keep the original tag stable through the lifecycle rather than changing it to a planned removal version.
|
||||
|
||||
See sample: `obsoletion-requires-reason-and-tag.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Setting only `ObsoleteState = Pending;` (or `Removed`) without `ObsoleteReason` and `ObsoleteTag`. Callers see a warning with no explanation, and the deprecation cannot be tracked by version.
|
||||
Setting only `ObsoleteState = Pending`/`Removed` on an object or field, or using `[Obsolete('', '')]` on a method, variable, or event. Both forms produce deprecation metadata without useful replacement guidance or traceability.
|
||||
|
||||
See sample: `obsoletion-requires-reason-and-tag.bad.al`.
|
||||
|
||||
|
|
|
|||
|
|
@ -16,5 +16,6 @@ codeunit 50213 "Upgrade Tag Registration"
|
|||
exit('MS-123456-MyFeature-20240101');
|
||||
end;
|
||||
|
||||
// No OnGetPerCompanyUpgradeTags subscriber — the tag is unknown to the platform.
|
||||
// No OnGetPerCompanyUpgradeTags subscriber: SetAllUpgradeTags cannot seed this
|
||||
// historical step for a newly initialized company, so it can run unnecessarily.
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,18 +1,6 @@
|
|||
codeunit 50212 "Upgrade Tag Registration"
|
||||
codeunit 50212 "Upgrade Tag Definitions"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
||||
trigger OnUpgradePerCompany()
|
||||
var
|
||||
UpgradeTag: Codeunit "Upgrade Tag";
|
||||
begin
|
||||
if UpgradeTag.HasUpgradeTag(MyUpgradeTag()) then
|
||||
exit;
|
||||
// Upgrade work ...
|
||||
UpgradeTag.SetUpgradeTag(MyUpgradeTag());
|
||||
end;
|
||||
|
||||
local procedure MyUpgradeTag(): Code[250]
|
||||
procedure MyUpgradeTag(): Code[250]
|
||||
begin
|
||||
exit('MS-123456-MyFeature-20240101');
|
||||
end;
|
||||
|
|
@ -23,3 +11,34 @@ codeunit 50212 "Upgrade Tag Registration"
|
|||
PerCompanyUpgradeTags.Add(MyUpgradeTag());
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50214 "Upgrade Tagged Feature"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
||||
trigger OnUpgradePerCompany()
|
||||
var
|
||||
UpgradeTag: Codeunit "Upgrade Tag";
|
||||
Tags: Codeunit "Upgrade Tag Definitions";
|
||||
begin
|
||||
if UpgradeTag.HasUpgradeTag(Tags.MyUpgradeTag()) then
|
||||
exit;
|
||||
// Upgrade work ...
|
||||
UpgradeTag.SetUpgradeTag(Tags.MyUpgradeTag());
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50215 "Install Tagged Feature"
|
||||
{
|
||||
Subtype = Install;
|
||||
|
||||
trigger OnInstallAppPerCompany()
|
||||
var
|
||||
UpgradeTag: Codeunit "Upgrade Tag";
|
||||
Tags: Codeunit "Upgrade Tag Definitions";
|
||||
begin
|
||||
// Existing-company install path; new-company initialization uses
|
||||
// SetAllUpgradeTags and the subscriber above.
|
||||
UpgradeTag.SetUpgradeTag(Tags.MyUpgradeTag());
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,22 +7,22 @@ countries: [w1]
|
|||
application-area: [all]
|
||||
---
|
||||
|
||||
# Register every upgrade tag with the platform via an event subscriber
|
||||
# Register upgrade tags that must be seeded for new companies
|
||||
|
||||
## Description
|
||||
|
||||
The `Upgrade Tag` codeunit only recognizes a tag if the tag was published to the platform through one of two events on that codeunit: `OnGetPerCompanyUpgradeTags` for tags set inside `OnUpgradePerCompany`, and `OnGetPerDatabaseUpgradeTags` for tags set inside `OnUpgradePerDatabase`. A tag that is `Set` and `Has`-checked in code but never added to one of these lists is unknown to the platform — its semantics around skip-on-reinstall, telemetry, and operator queries do not apply.
|
||||
`SetUpgradeTag(Tag)` directly records a completed per-company upgrade step; `HasUpgradeTag(Tag)` can then guard that step on later upgrades. The `OnGetPerCompanyUpgradeTags` subscriber serves a different path: it contributes tags to the list used by `SetAllUpgradeTags()` when a new company is initialized, marking historical upgrade steps complete so they do not run against a company that starts on the current schema.
|
||||
|
||||
The registration scope must match where the tag is set: a tag used from `OnUpgradePerCompany` registers in `OnGetPerCompanyUpgradeTags`; a tag used from `OnUpgradePerDatabase` registers in `OnGetPerDatabaseUpgradeTags`. Crossing the scopes silently breaks the tag.
|
||||
Registration is not install-time seeding. When an extension is installed into an existing company and a tag must start as complete, the install code must call `SetUpgradeTag` explicitly. For new-company initialization, codeunit `Company Initialize` calls `SetAllUpgradeTags`, which obtains subscriber-provided per-company tags and inserts missing ones. Database-scoped upgrade steps use `HasDatabaseUpgradeTag`/`SetDatabaseUpgradeTag` and the corresponding per-database list.
|
||||
|
||||
## Best Practice
|
||||
|
||||
For every new upgrade tag, add one line to the matching subscriber: `PerCompanyUpgradeTags.Add(MyUpgradeTag());` or `PerDatabaseUpgradeTags.Add(MyUpgradeTag());`. Place the subscribers in the same codeunit (or a dedicated "Upgrade Tag Definitions" codeunit) so the tag string and its registration stay together.
|
||||
In the upgrade codeunit, guard work with `HasUpgradeTag` and call `SetUpgradeTag` only after successful completion. Seed the same tag explicitly from `OnInstallAppPerCompany` when first-install logic should not run as a later upgrade. Also add historical per-company tags to `OnGetPerCompanyUpgradeTags` so `SetAllUpgradeTags` marks them complete for newly created companies. Keep the tag definition shared so all paths use the exact same value.
|
||||
|
||||
See sample: `register-upgrade-tags-with-subscribers.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `UpgradeTag.SetUpgradeTag(MyUpgradeTag())` without ever adding `MyUpgradeTag()` to the corresponding `OnGetPerCompany...` / `OnGetPerDatabase...` subscriber.
|
||||
Assuming an `OnGetPerCompanyUpgradeTags` subscriber sets tags during extension installation, or omitting the subscriber and allowing old upgrade steps to run when `SetAllUpgradeTags` initializes a new company. The subscriber supplies a list; only `SetAllUpgradeTags` or an explicit `SetUpgradeTag` call persists it.
|
||||
|
||||
See sample: `register-upgrade-tags-with-subscribers.bad.al`.
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
codeunit 50201 "Upgrade My Feature"
|
||||
{
|
||||
// Missing Subtype = Upgrade; the OnUpgrade trigger is never dispatched.
|
||||
trigger OnUpgradePerCompany()
|
||||
// This compiles, but no Subtype = Upgrade trigger wires it to the pipeline.
|
||||
procedure RunUpgrade()
|
||||
begin
|
||||
UpgradeMyFeature();
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
A codeunit only participates in the upgrade pipeline when it sets `Subtype = Upgrade`. The platform then dispatches the `OnUpgradePerCompany` and `OnUpgradePerDatabase` triggers on that codeunit during upgrade. A codeunit without `Subtype = Upgrade` — even one that declares an `OnUpgradePerCompany` trigger — is not an upgrade codeunit, and reviewers ignore it for upgrade concerns. Conversely, any procedure invoked transitively from an `OnUpgrade...` trigger of an upgrade codeunit IS upgrade code regardless of where it lives, and the upgrade rules apply to it.
|
||||
A codeunit only participates in the upgrade pipeline when it sets `Subtype = Upgrade`. The platform then permits and dispatches the `OnUpgradePerCompany` and `OnUpgradePerDatabase` triggers on that codeunit during upgrade. A normal codeunit can contain an upgrade-like `RunUpgrade` procedure, but the platform does not discover or invoke it automatically. Conversely, any procedure invoked transitively from an `OnUpgrade...` trigger of an upgrade codeunit is upgrade code regardless of where the helper lives, and the upgrade rules apply to it.
|
||||
|
||||
## Best Practice
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue