From 488ce50775bb1d94611191d2db83de35f7490763 Mon Sep 17 00:00:00 2001 From: Jesper Schulz-Wedde Date: Fri, 2 Oct 2026 11:35:11 +0200 Subject: [PATCH] knowledge(upgrade): upgrade code must not use ChangeCompany (#210) * knowledge(upgrade): upgrade code must not use ChangeCompany Addresses ADO bug 651092. Upgrade and feature data update code must run in the context of the company being upgraded; ChangeCompany leaves triggers, events and upgrade tags in the calling company and races the target company's own upgrade. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address review: self-contained samples, fixture registration, scoped feature routing - Declare the sample table in both companions so each compiles on its own. - Register no-changecompany-in-upgrade and changecompany-runs-triggers-in-the-calling-company in review-fixtures.json. - Limit the Feature Data Update rule to UpdateData/AfterUpdate and their reachable helpers; read-only IsDataUpdateRequired/ReviewData preflight is permitted and shown as a clean control. - Add feature data update execution to al-upgrade-review applicability and not-applicable scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Clarify cross-company upgrade sequencing without race claims Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Jesper Schulz-Wedde Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- evaluation/review-fixtures.json | 8 +- ...ny-runs-triggers-in-the-calling-company.md | 2 +- .../no-changecompany-in-upgrade.bad.al | 41 ++++++++ .../no-changecompany-in-upgrade.good.al | 96 +++++++++++++++++++ .../upgrade/no-changecompany-in-upgrade.md | 36 +++++++ microsoft/skills/review/al-upgrade-review.md | 11 ++- 6 files changed, 186 insertions(+), 8 deletions(-) create mode 100644 microsoft/knowledge/upgrade/no-changecompany-in-upgrade.bad.al create mode 100644 microsoft/knowledge/upgrade/no-changecompany-in-upgrade.good.al create mode 100644 microsoft/knowledge/upgrade/no-changecompany-in-upgrade.md diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index 974175f..307613f 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -37,7 +37,10 @@ ] }, "events": { - "article": "reset-ishandled-only-when-the-value-can-carry-over" + "articles": [ + "reset-ishandled-only-when-the-value-can-carry-over", + "changecompany-runs-triggers-in-the-calling-company" + ] }, "finance": { "articles": [ @@ -167,7 +170,8 @@ "upgrade": { "articles": [ "initvalue-does-not-update-existing-rows", - "upgrade-tag-logic-must-not-nest-deeply" + "upgrade-tag-logic-must-not-nest-deeply", + "no-changecompany-in-upgrade" ], "context": "The extended table existed in the previous app version and already contains rows." }, diff --git a/microsoft/knowledge/events/changecompany-runs-triggers-in-the-calling-company.md b/microsoft/knowledge/events/changecompany-runs-triggers-in-the-calling-company.md index 4a524f4..783fe81 100644 --- a/microsoft/knowledge/events/changecompany-runs-triggers-in-the-calling-company.md +++ b/microsoft/knowledge/events/changecompany-runs-triggers-in-the-calling-company.md @@ -13,7 +13,7 @@ application-area: [all] ## Description -`ChangeCompany` redirects the data access of one record variable to another company's table. Execution context does not move with it: Microsoft Learn states that triggers still run in the current company, not in the company passed to `ChangeCompany`. Code that knows this usually reaches for `Insert(false)` and copies the trigger's work by hand from the target company's setup. That closes only half of the gap. The runtime raises the database trigger events (`OnBeforeInsertEvent`, `OnAfterInsertEvent`, and their modify, delete, and rename counterparts) on every database operation and only passes the `RunTrigger` flag to the subscriber, so every subscriber that does not exit on `RunTrigger = false` still runs, in the calling company, against the calling company's setup, number series, and companion tables. The row lands in the target company, the side effects land in the caller, and nothing reports an error. The per-row cost of the call is a separate concern, see `changecompany-in-loop-drops-caches`. +`ChangeCompany` redirects the data access of one record variable to another company's table. Execution context does not move with it: Microsoft Learn states that triggers still run in the current company, not in the company passed to `ChangeCompany`. Code that knows this usually reaches for `Insert(false)` and copies the trigger's work by hand from the target company's setup. That closes only half of the gap. The runtime raises the database trigger events (`OnBeforeInsertEvent`, `OnAfterInsertEvent`, and their modify, delete, and rename counterparts) on every database operation and only passes the `RunTrigger` flag to the subscriber, so every subscriber that does not exit on `RunTrigger = false` still runs, in the calling company, against the calling company's setup, number series, and companion tables. The row lands in the target company, the side effects land in the caller, and nothing reports an error. The per-row cost of the call is a separate concern, see `changecompany-in-loop-drops-caches`. Upgrade code must not use `ChangeCompany` at all, see `no-changecompany-in-upgrade`. ## Best Practice diff --git a/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.bad.al b/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.bad.al new file mode 100644 index 0000000..44781a7 --- /dev/null +++ b/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.bad.al @@ -0,0 +1,41 @@ +table 50263 "Sales Order Ext" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) { } + field(2; "Shipping Agent Code"; Code[10]) { TableRelation = "Shipping Agent"; } + field(3; "Legacy Carrier Code"; Code[10]) { } + } + + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +codeunit 50261 "Upgrade All Companies" +{ + Subtype = Upgrade; + + trigger OnUpgradePerDatabase() + var + Company: Record Company; + SalesOrderExt: Record "Sales Order Ext"; + begin + // Reaches into every company from one session. Each company also has its own + // upgrade session, triggers and subscribers run in the calling context, and a + // data error in any company aborts the whole upgrade. + if Company.FindSet() then + repeat + SalesOrderExt.ChangeCompany(Company.Name); + SalesOrderExt.SetRange("Shipping Agent Code", ''); + if SalesOrderExt.FindSet(true) then + repeat + SalesOrderExt.Validate("Shipping Agent Code", SalesOrderExt."Legacy Carrier Code"); + SalesOrderExt.Modify(true); + until SalesOrderExt.Next() = 0; + until Company.Next() = 0; + end; +} diff --git a/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.good.al b/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.good.al new file mode 100644 index 0000000..c6374fc --- /dev/null +++ b/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.good.al @@ -0,0 +1,96 @@ +table 50262 "Sales Order Ext" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) { } + field(2; "Shipping Agent Code"; Code[10]) { TableRelation = "Shipping Agent"; } + field(3; "Legacy Carrier Code"; Code[10]) { } + } + + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +codeunit 50260 "Upgrade Current Company" +{ + Subtype = Upgrade; + + // The platform runs this trigger once per company, in that company's own session. + trigger OnUpgradePerCompany() + begin + UpgradeShippingAgentCodes(); + end; + + local procedure UpgradeShippingAgentCodes() + var + SalesOrderExt: Record "Sales Order Ext"; + UpgradeTag: Codeunit "Upgrade Tag"; + begin + if UpgradeTag.HasUpgradeTag(ShippingAgentUpgradeTag()) then + exit; + + // Only the current company's rows; triggers, events, and the tag all apply here. + SalesOrderExt.SetRange("Shipping Agent Code", ''); + if SalesOrderExt.FindSet(true) then + repeat + SalesOrderExt.Validate("Shipping Agent Code", SalesOrderExt."Legacy Carrier Code"); + SalesOrderExt.Modify(true); + until SalesOrderExt.Next() = 0; + + UpgradeTag.SetUpgradeTag(ShippingAgentUpgradeTag()); + end; + + local procedure ShippingAgentUpgradeTag(): Code[250] + begin + exit('CONTOSO-1001-ShippingAgentCode-20260101'); + end; +} + +codeunit 50264 "Shipping Agent Feat. Data Upd." implements "Feature Data Update" +{ + // Read-only preflight: counting rows across companies to report scope is allowed. + procedure IsDataUpdateRequired(): Boolean + var + Company: Record Company; + SalesOrderExt: Record "Sales Order Ext"; + begin + if Company.FindSet() then + repeat + SalesOrderExt.ChangeCompany(Company.Name); + SalesOrderExt.SetRange("Shipping Agent Code", ''); + if not SalesOrderExt.IsEmpty() then + exit(true); + until Company.Next() = 0; + exit(false); + end; + + procedure ReviewData() + begin + end; + + // Feature Management runs this once per company, in that company. + procedure UpdateData(FeatureDataUpdateStatus: Record "Feature Data Update Status") + var + SalesOrderExt: Record "Sales Order Ext"; + begin + SalesOrderExt.SetRange("Shipping Agent Code", ''); + if SalesOrderExt.FindSet(true) then + repeat + SalesOrderExt.Validate("Shipping Agent Code", SalesOrderExt."Legacy Carrier Code"); + SalesOrderExt.Modify(true); + until SalesOrderExt.Next() = 0; + end; + + procedure AfterUpdate(FeatureDataUpdateStatus: Record "Feature Data Update Status") + begin + end; + + procedure GetTaskDescription(): Text + begin + exit('Copies legacy carrier codes to the shipping agent code.'); + end; +} diff --git a/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.md b/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.md new file mode 100644 index 0000000..3295bba --- /dev/null +++ b/microsoft/knowledge/upgrade/no-changecompany-in-upgrade.md @@ -0,0 +1,36 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [changecompany, cross-company, onupgradepercompany, onupgradeperdatabase, feature-data-update, taskscheduler, multi-company, company-context] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Upgrade code must not use ChangeCompany + +## Description + +The platform upgrades each company in its own context: `OnUpgradePerCompany` (like the other `PerCompany` upgrade and install triggers) runs once per company, in a separate system session opened for that company, while `PerDatabase` triggers run in a session that opens no company. Calling `ChangeCompany()` in upgrade code reaches from the company being upgraded into another company. Cross-company work bypasses the platform's per-company execution boundary, making execution order and migration ownership difficult to reason about and potentially repeating work. A cross-company read cannot assume that the other company's migration has already run, so it can observe pre-upgrade or post-upgrade data. Per-company upgrade tags are set for the current company only, so a tag set after writing to company B is recorded for company A, which can cause B's own session to repeat the migration. Execution context does not move with `ChangeCompany`: table triggers and trigger-event subscribers still run in the calling company (see `changecompany-runs-triggers-in-the-calling-company`). Data and side effects then land in different companies, and an error in another company's data aborts the current company's upgrade. The same applies to the migration path of a feature-switch data update: the `UpdateData` and `AfterUpdate` methods of a `Feature Data Update` implementation. Feature Management runs them for one company's status row, either as a task scheduled in that company or in the current session. The interface's preflight methods, `IsDataUpdateRequired` and `ReviewData`, are a separate phase that reports scope before any update is scheduled. + +## Best Practice + +Upgrade code operates only on the company it is running in. Put per-company data migration in `OnUpgradePerCompany` (or a helper reachable only from it), guard it with a per-company upgrade tag, and let the platform invoke it for every company. Reserve `OnUpgradePerDatabase` for tables with `DataPerCompany = false` and other database-wide state; it needs no `ChangeCompany`. A feature data update implements `UpdateData` and `AfterUpdate` against the current company only. Its read-only preflight may use `ChangeCompany` to count or inspect rows in other companies, because it migrates nothing. When code outside the upgrade pipeline must start work in other companies, schedule it in each company with `TaskScheduler.CreateTask` and the company name, as Feature Management does, rather than writing there through `ChangeCompany`. The scheduled task runs in the target company, so triggers, events, permissions, and upgrade tags all use that company. + +See sample: [`no-changecompany-in-upgrade.good.al`](no-changecompany-in-upgrade.good.al). + +## Anti Pattern + +A loop over the `Company` table in `OnUpgradePerDatabase` or `OnUpgradePerCompany` that calls `ChangeCompany(Company.Name)` on a record or `RecordRef` and then reads, inserts, modifies, or deletes data. Another form is a `Feature Data Update` implementation whose `UpdateData` or `AfterUpdate` does the same to update every company from one task. On these paths reads are not exempt: they observe another company whose upgrade state is unknown. + +Detection signal: `Record.ChangeCompany()` or `RecordRef.ChangeCompany()` with a company-name argument in a codeunit with `Subtype = Upgrade` or in any procedure transitively reachable from its upgrade triggers, or in a `Feature Data Update` implementation's `UpdateData` or `AfterUpdate` or any procedure transitively reachable from them. Do not flag `ChangeCompany` in `IsDataUpdateRequired`, `ReviewData`, or helpers reachable only from them; a helper shared with `UpdateData` or `AfterUpdate` is in scope. Do not flag the parameterless `ChangeCompany()`, which only points the variable back at the current company, or `ChangeCompany` in ordinary runtime code; `changecompany-runs-triggers-in-the-calling-company` governs that. + +See sample: [`no-changecompany-in-upgrade.bad.al`](no-changecompany-in-upgrade.bad.al). + +## References + +- Upgrading extensions, Upgrade triggers: `PerCompany` triggers run once per company, each in its own system session for that company — https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-upgrading-extensions +- Record.ChangeCompany method, Remarks: triggers still run in the current company — https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-changecompany-method +- Interface "Feature Data Update" — https://learn.microsoft.com/en-us/dynamics365/business-central/application/system-application/interface/system.environment.configuration.feature-data-update +- `FeatureManagementImpl.UpdateData` calls the interface's `UpdateData` then `AfterUpdate`; `ReviewData` calls `IsDataUpdateRequired` and `ReviewData` — https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Feature%20Key/src/FeatureManagementImpl.Codeunit.al +- `FeatureManagementImpl.CreateTask` schedules `Update Feature Data` with `TaskScheduler.CreateTask` for the status row's company — https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Feature%20Key/src/FeatureManagementImpl.Codeunit.al diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index dce1053..f2c8b1d 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -16,7 +16,7 @@ application-area: [all] Reviews AL source changes against the `upgrade` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. -An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Upgrade findings are narrow by design — they apply when the review scope contains upgrade codeunits, install codeunits, table schema, enums, or objects under migration namespaces. The skill returns `not-applicable` when none of those apply. +An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Upgrade findings are narrow by design — they apply when the review scope contains upgrade codeunits, install codeunits, `Feature Data Update` implementations, table schema, enums, or objects under migration namespaces. The skill returns `not-applicable` when none of those apply. ## Source @@ -37,13 +37,14 @@ 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 `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`, `ModuleInfo`, `AppVersion`, `DataVersion`, `NavApp.GetCurrentModuleInfo`, `ExecutionContext`, `PrimaryKey`, `key(`, `field(`, `value(`, `enum`, `enumextension`, `HybridSL`, `HybridGP`, `HybridBC`, `HybridBaseDeployment`). +- The changed AL object names and types — especially codeunits with `Subtype = Upgrade` or `Subtype = Install`, codeunits implementing `Feature Data Update`, tables and tableextensions adding or changing fields, enums and enumextensions, and objects under `Hybrid*`/`Migration`/`Upgrade` namespaces. +- The changed triggers and procedures, weighted toward `OnCheckPreconditionsPerCompany`/`PerDatabase`, `OnUpgradePerCompany`/`PerDatabase`, `OnValidateUpgradePerCompany`/`PerDatabase`, `OnInstallAppPerCompany`/`PerDatabase`, the `OnGetPerCompanyUpgradeTags`/`OnGetPerDatabaseUpgradeTags` subscribers, the `UpdateData`/`AfterUpdate` methods of `Feature Data Update` implementations, 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`, `ModuleInfo`, `AppVersion`, `DataVersion`, `NavApp.GetCurrentModuleInfo`, `ExecutionContext`, `ChangeCompany`, `Feature Data Update`, `UpdateData`, `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. - Worklist `install-and-upgrade-codeunits-have-no-order.md` when a change adds multiple install or upgrade codeunits whose same-phase triggers share state or depend on one another. +- Worklist `no-changecompany-in-upgrade.md` when `ChangeCompany` with a company-name argument appears in an upgrade codeunit, in a helper reachable from its upgrade triggers, or in the `UpdateData` or `AfterUpdate` method of a `Feature Data Update` implementation or a helper reachable from them. Trace that reachability with the same call-graph and confidence rules as the check-only rule. Do not worklist it for `ChangeCompany` reachable only from `IsDataUpdateRequired` or `ReviewData`; that read-only preflight is permitted. - Worklist `appversion-meaning-depends-on-execution-context.md` when install or upgrade code branches on `ModuleInfo.AppVersion()` or confuses it with `DataVersion()`. - An upgrade tag's existence check (`HasUpgradeTag`) is nested inside another tag's guarded body, or one tagged procedure performs two or more functionally unrelated migrations (different tables, fields, or concerns) under a single tag, or one procedure mixes the gated logic for more than one distinct upgrade tag — `upgrade-tag-logic-must-not-nest-deeply.md`. Do not flag record loops or business-data safety guards (corruption checks, redundant-write checks, or other conditions) that serve the single migration the tag represents, however many `if` levels they take — that is the compliant shape the article explicitly permits. @@ -77,7 +78,7 @@ Outcome selection: - `completed` — the skill evaluated every worklist item. - `no-knowledge` — no applicable upgrade knowledge survived filtering. -- `not-applicable` — the diff touches no upgrade, install, schema, or enum surface. +- `not-applicable` — the diff touches no upgrade, install, feature data update, schema, or enum surface. - `partial` — a budget was hit before the worklist was exhausted. - `failed` — an unrecoverable error occurred.