mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Add lifecycle error and privacy knowledge (#99)
* Add lifecycle error and privacy knowledge Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95c06ad8-377d-4faa-8d07-06300b1c81ec * Fix lifecycle privacy review findings Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a26cd6d6-ff49-433e-bc53-f645c455ebdd * Refine lifecycle privacy retrieval Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95c06ad8-377d-4faa-8d07-06300b1c81ec * Make review gates explicit Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95c06ad8-377d-4faa-8d07-06300b1c81ec --------- Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
363f08f47e
commit
e0ebdd35c7
25 changed files with 459 additions and 39 deletions
|
|
@ -7,7 +7,6 @@ codeunit 50190 "Error Type Good Sample"
|
|||
if not BucketInitialized(BucketId) then begin
|
||||
InternalErr.ErrorType := ErrorType::Internal;
|
||||
InternalErr.Message := StrSubstNo('Ledger bucket %1 was not initialized before posting.', BucketId);
|
||||
InternalErr.DetailedMessage := 'Internal invariant violated. Inspect the call stack captured in telemetry.';
|
||||
Error(InternalErr);
|
||||
end;
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
bc-version: [14..]
|
||||
domain: error-handling
|
||||
keywords: [errorinfo, errortype, internal, client, telemetry, diagnostics, generic-message]
|
||||
technologies: [al]
|
||||
|
|
@ -15,7 +15,7 @@ application-area: [all]
|
|||
|
||||
## Best Practice
|
||||
|
||||
Reserve `ErrorType::Internal` for errors the user cannot act on: corrupted internal state, an unreachable branch, a contract a caller violated. Set a precise, detail-rich `Message` and `DetailedMessage` for telemetry, raise it via `Error(ErrorInfo)`, and let the platform show the user a generic dialog. Keep `ErrorType::Client` (or a plain `Error`) for failures the user is expected to read and resolve — validation messages, missing setup, business-rule violations. The test is simple: if the message only makes sense to a developer, mark it `Internal`.
|
||||
Reserve `ErrorType::Internal` for errors the user cannot act on: corrupted internal state, an unreachable branch, a contract a caller violated. Set a precise, detail-rich `Message` for telemetry, raise it via `Error(ErrorInfo)`, and let the platform show the user a generic dialog. Keep `ErrorType::Client` (or a plain `Error`) for failures the user is expected to read and resolve — validation messages, missing setup, business-rule violations. The test is simple: if the message only makes sense to a developer, mark it `Internal`.
|
||||
|
||||
See sample: `errortype-internal-vs-client-for-diagnostics.good.al`.
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,17 @@
|
|||
codeunit 50301 "Try Return Bad"
|
||||
{
|
||||
procedure ImportDocument()
|
||||
begin
|
||||
// Ignoring the Boolean result makes this an ordinary, throwing call.
|
||||
TryImportDocument();
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryImportDocument()
|
||||
begin
|
||||
Error(SourceRejectedErr);
|
||||
end;
|
||||
|
||||
var
|
||||
SourceRejectedErr: Label 'The source document was rejected.';
|
||||
}
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 50300 "Try Return Good"
|
||||
{
|
||||
procedure ImportDocument()
|
||||
begin
|
||||
if not TryImportDocument() then
|
||||
Error(ImportFailedErr);
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryImportDocument()
|
||||
begin
|
||||
Error(SourceRejectedErr);
|
||||
end;
|
||||
|
||||
var
|
||||
ImportFailedErr: Label 'The document could not be imported.';
|
||||
SourceRejectedErr: Label 'The source document was rejected.';
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [13..]
|
||||
domain: error-handling
|
||||
keywords: [tryfunction, try-method, boolean-return, ignored-return-value, error-propagation]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Consume a TryFunction return value to enable try semantics
|
||||
|
||||
## Description
|
||||
|
||||
A procedure marked `[TryFunction]` catches errors only when the caller uses its Boolean return value. An assignment or conditional makes the invocation a try-method call; a bare call is treated as an ordinary procedure call and exposes errors as usual. The attribute alone does not make every invocation non-throwing.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Consume the result directly: assign it to a Boolean or use the call in an `if` condition. Handle `false` immediately while the last-error state still describes that failure.
|
||||
|
||||
See sample: `ignored-tryfunction-return-disables-try-semantics.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling a `[TryFunction]` procedure as a standalone statement and assuming the attribute suppresses its errors. The call has ordinary error semantics because its Boolean result is ignored.
|
||||
|
||||
See sample: `ignored-tryfunction-return-disables-try-semantics.bad.al`.
|
||||
|
|
@ -0,0 +1,12 @@
|
|||
codeunit 50308 "ErrorInfo Privacy Bad"
|
||||
{
|
||||
procedure RaiseSynchronizationError(Customer: Record Customer)
|
||||
var
|
||||
FailureInfo: ErrorInfo;
|
||||
begin
|
||||
FailureInfo.Message := StrSubstNo('Synchronization failed for %1.', Customer."E-Mail");
|
||||
FailureInfo.DataClassification := DataClassification::SystemMetadata;
|
||||
FailureInfo.ErrorType := ErrorType::Internal;
|
||||
Error(FailureInfo);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 50307 "ErrorInfo Privacy Good"
|
||||
{
|
||||
procedure RaiseSynchronizationError()
|
||||
var
|
||||
FailureInfo: ErrorInfo;
|
||||
begin
|
||||
FailureInfo.Message := SynchronizationFailedErr;
|
||||
FailureInfo.DataClassification := DataClassification::SystemMetadata;
|
||||
FailureInfo.ErrorType := ErrorType::Client;
|
||||
Error(FailureInfo);
|
||||
end;
|
||||
|
||||
var
|
||||
SynchronizationFailedErr: Label 'The synchronization could not be completed.';
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [14..]
|
||||
domain: privacy
|
||||
keywords: [errorinfo, errorinfo-message, errorinfo-dataclassification, errorinfo-errortype, errorinfo-detailedmessage, copy-details, telemetry]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Review each ErrorInfo text surface by its actual exposure
|
||||
|
||||
## Description
|
||||
|
||||
Runtime 3.0 (BC 14) provides `ErrorInfo.Message`, `DataClassification`, and `ErrorType`. `Message` is sent to telemetry; with `ErrorType::Client` it is also the primary client message, while `ErrorType::Internal` replaces it in the client with a generic message but still sends the specified text to telemetry. `DataClassification` classifies the content in `Message`; it does not make incorrectly classified personal data safe. Runtime 8.0 (BC 19) adds `DetailedMessage`, which is omitted from the primary message but included in the error dialog's **Copy details** content.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Keep `Message` stable and classify its actual content. Choose `ErrorType` for client usability, not as a telemetry privacy boundary. On BC 19 and later, put only support-safe technical context in `DetailedMessage`, because a user can copy it from the dialog. The samples use only members available at the BC 14 article floor.
|
||||
|
||||
See sample: `errorinfo-telemetry-classification-and-errortype.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Marking a dynamic customer-bearing `Message` as `SystemMetadata`, or assuming `ErrorType::Internal` keeps it out of telemetry. On BC 19 and later, the same anti-pattern includes placing secrets or personal data in `DetailedMessage` because it is not the primary dialog text.
|
||||
|
||||
See sample: `errorinfo-telemetry-classification-and-errortype.bad.al`.
|
||||
|
|
@ -0,0 +1,25 @@
|
|||
codeunit 50310 "LogError Privacy Bad"
|
||||
{
|
||||
procedure SendInvoice()
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
CustomDimensions: Dictionary of [Text, Text];
|
||||
ErrorCallStack: Text;
|
||||
ErrorText: Text;
|
||||
begin
|
||||
if TrySendInvoice() then
|
||||
exit;
|
||||
|
||||
ErrorText := GetLastErrorText();
|
||||
ErrorCallStack := GetLastErrorCallStack();
|
||||
CustomDimensions.Add('Operation', 'SendInvoice');
|
||||
FeatureTelemetry.LogError('0000FT2', 'Invoice exchange', 'Sending invoice',
|
||||
ErrorText, ErrorCallStack, CustomDimensions);
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TrySendInvoice()
|
||||
begin
|
||||
Error('Invoice %1 for %2 could not be sent.', 'INV-1001', 'user@example.com');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
codeunit 50309 "LogError Privacy Good"
|
||||
{
|
||||
procedure SendInvoice()
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
CustomDimensions: Dictionary of [Text, Text];
|
||||
ErrorCallStack: Text;
|
||||
ErrorText: Text;
|
||||
begin
|
||||
if TrySendInvoice() then
|
||||
exit;
|
||||
|
||||
ErrorText := GetLastErrorText(true);
|
||||
ErrorCallStack := GetLastErrorCallStack();
|
||||
CustomDimensions.Add('Operation', 'SendInvoice');
|
||||
FeatureTelemetry.LogError('0000FT1', 'Invoice exchange', 'Sending invoice',
|
||||
ErrorText, ErrorCallStack, CustomDimensions);
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TrySendInvoice()
|
||||
begin
|
||||
Error(SendFailedErr);
|
||||
end;
|
||||
|
||||
var
|
||||
SendFailedErr: Label 'The invoice could not be sent.';
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [18..]
|
||||
domain: privacy
|
||||
keywords: [featuretelemetry, logerror, errortext, errorcallstack, alerrortext, alerrorcallstack, customdimensions]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# FeatureTelemetry.LogError emits more than caller custom dimensions
|
||||
|
||||
## Description
|
||||
|
||||
`FeatureTelemetry.LogError` emits its `ErrorText` as the telemetry message and adds it as `alErrorText`. The overloads with `ErrorCallStack` also add `alErrorCallStack`. These dimensions are produced in addition to the caller-supplied `CustomDimensions` dictionary, and the Feature Telemetry implementation sends the event as `SystemMetadata`.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Review the dedicated error arguments as telemetry payload. Capture `GetLastErrorText(true)` when scrubbed platform error text is sufficient, and pass `GetLastErrorCallStack()` only as a call stack. Keep custom dimensions non-personal too.
|
||||
|
||||
See sample: `featuretelemetry-logerror-implicit-errortext.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Approving a `LogError` call because its explicit dictionary contains only safe values while it passes unsanitized `GetLastErrorText()` or arbitrary context through `ErrorText` or `ErrorCallStack`. Those arguments become telemetry dimensions outside the dictionary.
|
||||
|
||||
See sample: `featuretelemetry-logerror-implicit-errortext.bad.al`.
|
||||
|
|
@ -0,0 +1,32 @@
|
|||
codeunit 50303 "Upgrade Phases Bad"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
||||
trigger OnCheckPreconditionsPerCompany()
|
||||
begin
|
||||
// A precondition check must not repair the data it is checking.
|
||||
RenamePostingGroup();
|
||||
end;
|
||||
|
||||
trigger OnValidateUpgradePerCompany()
|
||||
begin
|
||||
// Validation must not perform a migration omitted from OnUpgrade.
|
||||
MigrateCustomerPostingGroups();
|
||||
end;
|
||||
|
||||
local procedure RenamePostingGroup()
|
||||
var
|
||||
CustomerPostingGroup: Record "Customer Posting Group";
|
||||
begin
|
||||
if CustomerPostingGroup.Get('OLD') then
|
||||
CustomerPostingGroup.Rename('NEW');
|
||||
end;
|
||||
|
||||
local procedure MigrateCustomerPostingGroups()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetRange("Customer Posting Group", 'OLD');
|
||||
Customer.ModifyAll("Customer Posting Group", 'NEW');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,63 @@
|
|||
codeunit 50302 "Upgrade Phases Good"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
||||
trigger OnCheckPreconditionsPerCompany()
|
||||
begin
|
||||
CheckTargetPostingGroup();
|
||||
end;
|
||||
|
||||
trigger OnUpgradePerCompany()
|
||||
var
|
||||
UpgradeTag: Codeunit "Upgrade Tag";
|
||||
begin
|
||||
if UpgradeTag.HasUpgradeTag(CustomerPostingGroupTag()) then
|
||||
exit;
|
||||
|
||||
MigrateCustomerPostingGroups();
|
||||
UpgradeTag.SetUpgradeTag(CustomerPostingGroupTag());
|
||||
end;
|
||||
|
||||
trigger OnValidateUpgradePerCompany()
|
||||
begin
|
||||
CheckLegacyPostingGroupsRemoved();
|
||||
end;
|
||||
|
||||
local procedure CheckTargetPostingGroup()
|
||||
var
|
||||
CustomerPostingGroup: Record "Customer Posting Group";
|
||||
begin
|
||||
if not CustomerPostingGroup.Get('NEW') then
|
||||
Error(TargetGroupMissingErr);
|
||||
end;
|
||||
|
||||
local procedure MigrateCustomerPostingGroups()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetRange("Customer Posting Group", 'OLD');
|
||||
if Customer.FindSet(true) then
|
||||
repeat
|
||||
Customer.Validate("Customer Posting Group", 'NEW');
|
||||
Customer.Modify(true);
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
|
||||
local procedure CheckLegacyPostingGroupsRemoved()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetRange("Customer Posting Group", 'OLD');
|
||||
if not Customer.IsEmpty() then
|
||||
Error(MigrationIncompleteErr);
|
||||
end;
|
||||
|
||||
local procedure CustomerPostingGroupTag(): Code[250]
|
||||
begin
|
||||
exit('MS-50302-CustomerPostingGroup-20260714');
|
||||
end;
|
||||
|
||||
var
|
||||
MigrationIncompleteErr: Label 'The legacy customer posting group was not migrated.';
|
||||
TargetGroupMissingErr: Label 'Customer posting group NEW must exist before the upgrade.';
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: upgrade
|
||||
keywords: [on-check-preconditions, on-validate-upgrade, on-upgrade, read-only-check, data-migration]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Upgrade check triggers do not migrate data
|
||||
|
||||
## Description
|
||||
|
||||
`OnCheckPreconditionsPerCompany`/`PerDatabase` run before the upgrade to verify that it can start. `OnValidateUpgradePerCompany`/`PerDatabase` run after upgrade logic to verify that it succeeded. Treat both phases as read-only checks. The `OnUpgradePerCompany`/`PerDatabase` phase is where the platform expects actual data transformation.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Have check triggers call query-only helpers that raise an error when an invariant fails. Put every `Insert`, `Modify`, `Delete`, `Rename`, `DataTransfer`, and other migration write behind helpers called from the matching `OnUpgrade...` trigger.
|
||||
|
||||
See sample: `check-only-triggers-do-not-migrate-data.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Repairing data in `OnCheckPreconditions...` or finishing migration in `OnValidateUpgrade...`. Those writes blur the phase contract and make a check alter the state it is supposed to assess.
|
||||
|
||||
See sample: `check-only-triggers-do-not-migrate-data.bad.al`.
|
||||
|
|
@ -4,8 +4,7 @@ codeunit 50211 "Install My Extension"
|
|||
|
||||
trigger OnInstallAppPerCompany()
|
||||
begin
|
||||
// No DataVersion() guard — this runs on every reinstall and upgrade
|
||||
// path, duplicating seed rows.
|
||||
// No DataVersion() guard: a reinstall duplicates seed rows.
|
||||
SeedDefaultRows();
|
||||
end;
|
||||
|
||||
|
|
|
|||
|
|
@ -11,20 +11,21 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
On the first install of an extension on a tenant the platform records a zero data version: `AppInfo.DataVersion()` returns `Version.Create('0.0.0.0')`. Subsequent upgrades record the actual previous version. The `OnInstallAppPerCompany` trigger uses this distinction to detect a brand-new install — for example, to seed default rows that should not be re-inserted on a normal upgrade. This is the one place where reading `DataVersion()` is the right tool; for everything else, use an upgrade tag.
|
||||
On the first install of an extension on a tenant the platform records a zero data version: `AppInfo.DataVersion()` returns `Version.Create('0.0.0.0')`. During reinstall, `DataVersion()` identifies the previously installed data version. The `OnInstallAppPerCompany` trigger uses this distinction to separate a brand-new install from a reinstall. Ordinary version upgrades do not run install code.
|
||||
|
||||
## Best Practice
|
||||
|
||||
In `OnInstallAppPerCompany`, fetch the current `ModuleInfo` via `NavApp.GetCurrentModuleInfo`, compare `AppInfo.DataVersion()` to `Version.Create('0.0.0.0')`, and run install-only seed logic only when they match. On any non-zero data version, exit immediately — that path is an upgrade, not an install.
|
||||
In `OnInstallAppPerCompany`, fetch the current `ModuleInfo` via `NavApp.GetCurrentModuleInfo`, compare `AppInfo.DataVersion()` to `Version.Create('0.0.0.0')`, and run first-install seed logic only when they match. On a non-zero data version, follow the reinstall path or exit.
|
||||
|
||||
See sample: `first-install-dataversion-zero-check.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Treating `OnInstallAppPerCompany` as if it always implies "fresh tenant". The trigger also fires when reinstalling over an existing data set; without the `0.0.0.0` guard, install-only seed code re-runs on every upgrade and duplicates rows.
|
||||
Treating `OnInstallAppPerCompany` as if it always implies "fresh tenant". The trigger also fires when reinstalling over an existing data set; without the `0.0.0.0` guard, first-install seed code can run again and duplicate rows.
|
||||
|
||||
See sample: `first-install-dataversion-zero-check.bad.al`.
|
||||
|
||||
## See also
|
||||
|
||||
- `use-upgrade-tags-not-version-checks.md` — for upgrade steps after first install, use upgrade tags rather than `DataVersion`.
|
||||
- `install-code-does-not-run-on-version-upgrade.md` — ordinary version upgrades invoke upgrade code, not install code.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,20 @@
|
|||
codeunit 50306 "My App Install Only"
|
||||
{
|
||||
Subtype = Install;
|
||||
|
||||
trigger OnInstallAppPerCompany()
|
||||
begin
|
||||
// A normal version upgrade never invokes this migration.
|
||||
MigrateLegacySetup();
|
||||
end;
|
||||
|
||||
local procedure MigrateLegacySetup()
|
||||
var
|
||||
MyAppSetup: Record "My App Setup";
|
||||
begin
|
||||
if MyAppSetup.Get() then begin
|
||||
MyAppSetup."Configuration Version" := 2;
|
||||
MyAppSetup.Modify(true);
|
||||
end;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,48 @@
|
|||
codeunit 50304 "My App Install"
|
||||
{
|
||||
Subtype = Install;
|
||||
|
||||
trigger OnInstallAppPerCompany()
|
||||
begin
|
||||
InitializeSetup();
|
||||
end;
|
||||
|
||||
local procedure InitializeSetup()
|
||||
var
|
||||
MyAppSetup: Record "My App Setup";
|
||||
begin
|
||||
if MyAppSetup.IsEmpty() then
|
||||
MyAppSetup.Insert(true);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50305 "My App Upgrade"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
||||
trigger OnUpgradePerCompany()
|
||||
var
|
||||
UpgradeTag: Codeunit "Upgrade Tag";
|
||||
begin
|
||||
if UpgradeTag.HasUpgradeTag(ConfigurationVersionTag()) then
|
||||
exit;
|
||||
|
||||
MigrateLegacySetup();
|
||||
UpgradeTag.SetUpgradeTag(ConfigurationVersionTag());
|
||||
end;
|
||||
|
||||
local procedure MigrateLegacySetup()
|
||||
var
|
||||
MyAppSetup: Record "My App Setup";
|
||||
begin
|
||||
if MyAppSetup.Get() then begin
|
||||
MyAppSetup."Configuration Version" := 2;
|
||||
MyAppSetup.Modify(true);
|
||||
end;
|
||||
end;
|
||||
|
||||
local procedure ConfigurationVersionTag(): Code[250]
|
||||
begin
|
||||
exit('MS-50305-ConfigurationVersion-20260714');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: upgrade
|
||||
keywords: [install-codeunit, subtype-install, on-install-app, version-upgrade, upgrade-codeunit]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Install code does not run during a version upgrade
|
||||
|
||||
## Description
|
||||
|
||||
An install codeunit runs when an extension is installed for the first time or an uninstalled version is installed again. Installing a higher extension version through the data-upgrade operation does not invoke `OnInstallAppPerCompany` or `OnInstallAppPerDatabase`. Ordinary version-to-version migration is dispatched only through upgrade codeunits.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `Subtype = Install` for first-install and reinstall initialization. Put version migration in a separate `Subtype = Upgrade` codeunit and enter it from `OnUpgradePerCompany` or `OnUpgradePerDatabase`.
|
||||
|
||||
See sample: `install-code-does-not-run-on-version-upgrade.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Putting a schema or data migration only in an install trigger and expecting it to run when a higher app version is upgraded. The migration is never invoked on that path.
|
||||
|
||||
See sample: `install-code-does-not-run-on-version-upgrade.bad.al`.
|
||||
|
|
@ -4,10 +4,21 @@ codeunit 50235 "Upgrade With Validation"
|
|||
|
||||
trigger OnValidateUpgradePerCompany()
|
||||
begin
|
||||
// No skip logic and no written justification — full-table validation
|
||||
// runs on every single upgrade pass.
|
||||
// A full-table scan repeats on every upgrade.
|
||||
ValidateAllCustomers();
|
||||
end;
|
||||
|
||||
local procedure ValidateAllCustomers() begin end;
|
||||
local procedure ValidateAllCustomers()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
if Customer."Customer Posting Group" = 'OLD' then
|
||||
Error(MigrationIncompleteErr);
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
|
||||
var
|
||||
MigrationIncompleteErr: Label 'The legacy customer posting group was not migrated.';
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3,23 +3,19 @@ codeunit 50234 "Upgrade With Validation"
|
|||
Subtype = Upgrade;
|
||||
|
||||
trigger OnValidateUpgradePerCompany()
|
||||
begin
|
||||
CheckNoLegacyPostingGroups();
|
||||
end;
|
||||
|
||||
local procedure CheckNoLegacyPostingGroups()
|
||||
var
|
||||
UpgradeTag: Codeunit "Upgrade Tag";
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
// Justification: regulatory compliance requires a full-table scan once
|
||||
// per tenant after this release. Tag prevents re-runs.
|
||||
if UpgradeTag.HasUpgradeTag(MyValidationUpgradeTag()) then
|
||||
exit;
|
||||
|
||||
ValidateAllCustomers();
|
||||
|
||||
UpgradeTag.SetUpgradeTag(MyValidationUpgradeTag());
|
||||
Customer.SetRange("Customer Posting Group", 'OLD');
|
||||
if not Customer.IsEmpty() then
|
||||
Error(MigrationIncompleteErr);
|
||||
end;
|
||||
|
||||
local procedure ValidateAllCustomers() begin end;
|
||||
|
||||
local procedure MyValidationUpgradeTag(): Code[250]
|
||||
begin
|
||||
exit('MS-123456-CustomerValidation-20240101');
|
||||
end;
|
||||
var
|
||||
MigrationIncompleteErr: Label 'The legacy customer posting group was not migrated.';
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,26 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: upgrade
|
||||
keywords: [on-validate-upgrade-per-company, performance-impact, skip-logic, justification, upgrade-tag]
|
||||
keywords: [on-validate-upgrade-per-company, performance-impact, bounded-query, justification, read-only-check]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Performance-impacting upgrade triggers need justification and skip logic
|
||||
# Keep upgrade validation checks bounded
|
||||
|
||||
## Description
|
||||
|
||||
Triggers such as `OnValidateUpgradePerCompany` run on every upgrade pass. When their body performs non-trivial work — full-table scans, cross-table validations — the cost is paid on every upgrade of every tenant, even when there is nothing to validate. That cost is acceptable only when the validation is critical (regulatory compliance, data-integrity guarantees the platform depends on) AND the trigger short-circuits once it has done its work.
|
||||
Triggers such as `OnValidateUpgradePerCompany` run on every upgrade pass. A full-table scan or cross-table validation therefore adds cost to every upgrade of every tenant. Validation is a read-only lifecycle check, so it cannot make itself one-time by writing an upgrade tag.
|
||||
|
||||
## Best Practice
|
||||
|
||||
A performance-impacting upgrade trigger carries two things: a written comment that names the reason the work has to happen on every upgrade pass, and an early-exit guard backed by an upgrade tag so the work runs at most once per tenant. The `HasUpgradeTag` check at the top exits when the validation has already been recorded; the `SetUpgradeTag` call at the bottom records completion.
|
||||
Filter directly to invalid rows and use `IsEmpty` or another bounded existence check where possible. If a broad validation is unavoidable, document the invariant that requires it and keep all data changes in `OnUpgrade...`.
|
||||
|
||||
See sample: `minimize-onvalidate-upgrade-triggers.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Doing real work in `OnValidateUpgradePerCompany` with no upgrade-tag guard. The same scan runs every upgrade, multiplying upgrade time by the number of releases the customer takes.
|
||||
Reading every record in `OnValidateUpgradePerCompany` when a filtered existence check can prove the same invariant. The scan repeats on every upgrade.
|
||||
|
||||
See sample: `minimize-onvalidate-upgrade-triggers.bad.al`.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue