diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al index 9791909..2dcd2d2 100644 --- a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al @@ -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; diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md index 7264ad6..127fa50 100644 --- a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md @@ -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`. diff --git a/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.bad.al b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.bad.al new file mode 100644 index 0000000..8d6df99 --- /dev/null +++ b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.bad.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.'; +} diff --git a/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.good.al b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.good.al new file mode 100644 index 0000000..d369d27 --- /dev/null +++ b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.good.al @@ -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.'; +} diff --git a/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md new file mode 100644 index 0000000..f61553d --- /dev/null +++ b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md @@ -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`. diff --git a/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.bad.al b/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.bad.al new file mode 100644 index 0000000..6fc5b06 --- /dev/null +++ b/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.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; +} diff --git a/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.good.al b/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.good.al new file mode 100644 index 0000000..1da3c1f --- /dev/null +++ b/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.good.al @@ -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.'; +} diff --git a/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.md b/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.md new file mode 100644 index 0000000..ce1b684 --- /dev/null +++ b/microsoft/knowledge/privacy/errorinfo-telemetry-classification-and-errortype.md @@ -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`. diff --git a/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.bad.al b/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.bad.al new file mode 100644 index 0000000..6906077 --- /dev/null +++ b/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.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; +} diff --git a/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.good.al b/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.good.al new file mode 100644 index 0000000..cae02be --- /dev/null +++ b/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.good.al @@ -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.'; +} diff --git a/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.md b/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.md new file mode 100644 index 0000000..863f3a8 --- /dev/null +++ b/microsoft/knowledge/privacy/featuretelemetry-logerror-implicit-errortext.md @@ -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`. diff --git a/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.bad.al b/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.bad.al new file mode 100644 index 0000000..af1dabf --- /dev/null +++ b/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.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; +} diff --git a/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.good.al b/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.good.al new file mode 100644 index 0000000..6929741 --- /dev/null +++ b/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.good.al @@ -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.'; +} diff --git a/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.md b/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.md new file mode 100644 index 0000000..30f6ca7 --- /dev/null +++ b/microsoft/knowledge/upgrade/check-only-triggers-do-not-migrate-data.md @@ -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`. diff --git a/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.bad.al b/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.bad.al index e3381ad..da501e6 100644 --- a/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.bad.al +++ b/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.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; diff --git a/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.md b/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.md index 260b15b..6324836 100644 --- a/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.md +++ b/microsoft/knowledge/upgrade/first-install-dataversion-zero-check.md @@ -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. diff --git a/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.bad.al b/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.bad.al new file mode 100644 index 0000000..bb19d21 --- /dev/null +++ b/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.bad.al @@ -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; +} diff --git a/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.good.al b/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.good.al new file mode 100644 index 0000000..c77f3cf --- /dev/null +++ b/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.good.al @@ -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; +} diff --git a/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.md b/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.md new file mode 100644 index 0000000..12f432f --- /dev/null +++ b/microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.md @@ -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`. diff --git a/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.bad.al b/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.bad.al index 69994e6..35b848b 100644 --- a/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.bad.al +++ b/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.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.'; } diff --git a/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.good.al b/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.good.al index 9a5a83b..8680775 100644 --- a/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.good.al +++ b/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.good.al @@ -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.'; } diff --git a/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.md b/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.md index 2c02def..b9e5e13 100644 --- a/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.md +++ b/microsoft/knowledge/upgrade/minimize-onvalidate-upgrade-triggers.md @@ -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`. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index 91228aa..50bd9e9 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -38,8 +38,9 @@ Discard files that are not applicable. Retain conditionally applicable files (an Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: - The changed AL object names and types — especially codeunits that post or validate, tables and table extensions with `OnValidate` triggers, and any procedure that raises errors or orchestrates a batch over records. -- The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]`. -- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`). +- The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]` or `[TryFunction]`. +- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`, `TryFunction`, `GetLastErrorText`, Boolean assignment). +- Resolve changed standalone call targets; when the target declaration has `[TryFunction]`, worklist the ignored-return rule even if the declaration itself is unchanged. Only assignment and conditional use activate try semantics. A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index bf60f5d..2cd4005 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -38,10 +38,12 @@ Discard files that are not applicable. Retain conditionally applicable files (an Narrow the relevant files to the subset that applies to the changes under review. Exclude test codeunits, test libraries, test helper code, files under test/Test/Tests paths, and objects with `Subtype = Test`; test data is synthetic and does not ship to customers. For each relevant file, compute overlap against: - The changed AL object names and types — especially tables and tableextensions (for `DataClassification` on fields), codeunits that call `Error`, `Session.LogMessage`, or `FeatureTelemetry`, codeunits performing outgoing HTTP requests with customer data, migration codeunits, and objects reading or writing `IsolatedStorage`. -- The changed procedures and triggers, weighted toward those that call `Error`, `Session.LogMessage`, `StrSubstNo`, `GetLastErrorText`, `FeatureTelemetry.LogUsage`/`LogUptake`/`LogError`, `HttpClient.Post`/`Get`, `IsolatedStorage.Set`/`SetEncrypted`/`Get`, or `PrivacyNotice.GetPrivacyNoticeApprovalState`. -- Tokens extracted from the diff that relate to privacy (`DataClassification`, `CustomerContent`, `EndUserIdentifiableInformation`, `EndUserPseudonymousIdentifiers`, `SystemMetadata`, `ToBeClassified`, `PrivacyNotice`, `GetLastErrorText`, `TelemetryScope`, `FeatureTelemetry`, `CustomDimensions`, `LogUsage`, `LogUptake`, `LogError`, `HybridSL`, `HybridGP`, `HybridBC`). +- The changed procedures and triggers, weighted toward those that call `Error`, construct `ErrorInfo`, call `Session.LogMessage`, `StrSubstNo`, `GetLastErrorText`/`GetLastErrorCallStack`, `FeatureTelemetry.LogUsage`/`LogUptake`/`LogError`, `HttpClient.Post`/`Get`, `IsolatedStorage.Set`/`SetEncrypted`/`Get`, or `PrivacyNotice.GetPrivacyNoticeApprovalState`. +- Tokens extracted from the diff that relate to privacy (`DataClassification`, `CustomerContent`, `EndUserIdentifiableInformation`, `EndUserPseudonymousIdentifiers`, `SystemMetadata`, `ToBeClassified`, `PrivacyNotice`, `ErrorInfo`, `GetLastErrorText`, `GetLastErrorCallStack`, `TelemetryScope`, `FeatureTelemetry`, `CustomDimensions`, `LogUsage`, `LogUptake`, `LogError`, `ErrorText`, `ErrorCallStack`, `alErrorText`, `alErrorCallStack`, `HybridSL`, `HybridGP`, `HybridBC`). +- Treat `ErrorInfo.Message`, `ErrorInfo.DataClassification`, `ErrorInfo.ErrorType`, and `ErrorInfo.DetailedMessage` as qualified member signals: accept a call or assignment only when symbol resolution proves that its receiver expression or variable has type `ErrorInfo`. Normalize those accesses to `errorinfo-message`, `errorinfo-dataclassification`, `errorinfo-errortype`, and `errorinfo-detailedmessage` retrieval tokens. Bare `Message` or `DataClassification` tokens MUST NOT trigger this article; do not emit the qualified tokens for `Message(...)` dialog calls, table or table-field `DataClassification` properties, or similarly named members on other types. Resolve the receiver's declaration from the containing object when it is outside the changed hunk. +- Worklist ErrorInfo privacy guidance only from those typed `ErrorInfo` member tokens or from construction of an `ErrorInfo` value. For every `FeatureTelemetry.LogError`, inspect the dedicated error text and call-stack arguments in addition to explicit custom dimensions. -A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. +A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Apply the topic-specific gates above after this overlap check; in particular, bare `Message` and `DataClassification` tokens cannot admit ErrorInfo guidance. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`. diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 3b91d67..c087585 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -38,8 +38,11 @@ Discard files that are not applicable. Retain conditionally applicable files (an Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: - The changed AL object names and types — especially codeunits with `Subtype = Upgrade` or `Subtype = Install`, tables and tableextensions adding or changing fields, enums and enumextensions, and objects under `Hybrid*`/`Migration`/`Upgrade` namespaces. -- The changed triggers and procedures, weighted toward `OnUpgradePerCompany`, `OnUpgradePerDatabase`, `OnValidateUpgradePerCompany`, `OnValidateUpgradePerDatabase`, `OnInstallAppPerCompany`, and the `OnGetPerCompanyUpgradeTags`/`OnGetPerDatabaseUpgradeTags` subscribers. -- Tokens extracted from the diff that relate to upgrade concerns (`Subtype = Upgrade`, `Upgrade Tag`, `HasUpgradeTag`, `SetUpgradeTag`, `OnValidateUpgrade`, `DataTransfer`, `CopyFields`, `InitValue`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `DataVersion`, `ExecutionContext`, `PrimaryKey`, `key(`, `field(`, `value(`, `enum`, `enumextension`, `HybridSL`, `HybridGP`, `HybridBC`, `HybridBaseDeployment`). +- The changed triggers and procedures, weighted toward `OnCheckPreconditionsPerCompany`/`PerDatabase`, `OnUpgradePerCompany`/`PerDatabase`, `OnValidateUpgradePerCompany`/`PerDatabase`, `OnInstallAppPerCompany`/`PerDatabase`, the `OnGetPerCompanyUpgradeTags`/`OnGetPerDatabaseUpgradeTags` subscribers, and helper procedures transitively reachable from those entry points. +- Tokens extracted from the diff that relate to upgrade concerns (`Subtype = Upgrade`, `Subtype = Install`, `Upgrade Tag`, `HasUpgradeTag`, `SetUpgradeTag`, `OnCheckPreconditions`, `OnUpgrade`, `OnValidateUpgrade`, `OnInstallApp`, `DataTransfer`, `CopyFields`, `Insert`, `Modify`, `Delete`, `Rename`, `InitValue`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `DataVersion`, `ExecutionContext`, `PrimaryKey`, `key(`, `field(`, `value(`, `enum`, `enumextension`, `HybridSL`, `HybridGP`, `HybridBC`, `HybridBaseDeployment`). +- For each `OnCheckPreconditions...` and `OnValidateUpgrade...` trigger, build the best available call graph from surrounding unchanged source as well as changed hunks, tracing resolved calls through reachable local or internal helpers. Worklist the check-only rule when a database write occurs either directly in the trigger or in any helper procedure reachable from it. Writes include `Insert`, `Modify`, `ModifyAll`, `Delete`, `DeleteAll`, `Rename`, and `DataTransfer`. Also perform the reverse check when a PR changes a writing helper body: worklist the rule when that helper is invoked directly or transitively by an unchanged check or validation trigger. +- Treat a direct write or a fully resolved call chain as high-confidence evidence. When cross-object dispatch, unavailable declarations, or an incomplete call graph prevents proving the complete chain, cap confidence at `medium`, name the unresolved edge in the finding, and do not claim a violation without a resolved path from a check or validation trigger to a write. +- Worklist the install-versus-upgrade rule when migration helpers are reachable only from an install codeunit. A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no upgrade-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files. @@ -57,7 +60,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice` Set `confidence` to: -- `high` when the detection is based on an unambiguous pattern match. +- `high` when the detection is based on an unambiguous pattern match and any required helper reachability is fully established. - `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. - `low` when the finding is an advisory derived only from applicability.