Merge 0561b12c96 into 35d0966a8d

evaluation/review-fixtures.json

@@ -17,7 +17,8 @@
       "article": "set-defaultimplementation-on-enum"
     },
     "performance": {
-      "article": "use-isempty-for-existence-check"
+      "article": "avoid-commit-inside-loops",
+      "context": "A single worker applies a one-time credit-limit increase over a stable customer set. Failed runs may be retried for the same operation."
     },
     "privacy": {
       "article": "no-pii-in-telemetry-message-string"
@@ -29,7 +30,8 @@
       "article": "telemetry-event-id-stable-unique"
     },
     "testing": {
-      "article": "ui-handlers-in-tests"
+      "article": "ui-handlers-in-tests",
+      "context": "ConfirmPostingAndShowMessage must verify exactly one 'Post this document?' confirmation, reply true, then exactly one 'Posting completed.' message in that order. The Customer Card scenarios verify the selected customer's identity; a credit-limit notification is conditional."
     },
     "upgrade": {
       "article": "initvalue-does-not-update-existing-rows",

microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al

@@ -1,26 +1,61 @@
+query 50127 "Perf Customer Chunk"
+{
+    QueryType = Normal;
+    OrderBy = ascending(CustomerNo);
+
+    elements
+    {
+        dataitem(Customer; Customer)
+        {
+            column(CustomerNo; "No.") { }
+        }
+    }
+}
+
 codeunit 50129 "Perf Sample CommitInLoop Bad"
 {
-    procedure NormalizeCustomerNames()
+    procedure IncreaseCustomerCreditLimits()
+    var
+        LastCustomerNo: Code[20];
+    begin
+        while IncreaseNextChunk(LastCustomerNo) do
+            Commit();
+    end;
+
+    local procedure IncreaseNextChunk(var LastCustomerNo: Code[20]): Boolean
     var
         Customer: Record Customer;
-        LastCustomerNo: Code[20];
+        TempCustomer: Record Customer temporary;
-        ProcessedCount: Integer;
+        CustomerChunk: Query "Perf Customer Chunk";
+        ChunkStartedAt: DateTime;
+        MaxChunkDuration: Duration;
     begin
-        Customer.SetFilter("No.", '>%1', LastCustomerNo);
+        ChunkStartedAt := CurrentDateTime();
-        if Customer.FindSet(true) then
+        MaxChunkDuration := 60000;
+        CustomerChunk.TopNumberOfRows(500);
+        if LastCustomerNo <> '' then
+            CustomerChunk.SetFilter(CustomerNo, '>%1', LastCustomerNo);
+        CustomerChunk.Open();
+        while CustomerChunk.Read() do begin
+            TempCustomer.Init();
+            TempCustomer."No." := CustomerChunk.CustomerNo;
+            TempCustomer.Insert();
+        end;
+        CustomerChunk.Close();
+
+        if TempCustomer.IsEmpty() then
+            exit(false);
+
+        Customer.LockTable();
+        if TempCustomer.FindSet() then
             repeat
-                Customer.Name := UpperCase(Customer.Name);
+                if Customer.Get(TempCustomer."No.") then begin
+                    Customer."Credit Limit (LCY)" += 100;
                     Customer.Modify();
+                end;
+                LastCustomerNo := TempCustomer."No.";
+            until (TempCustomer.Next() = 0) or (CurrentDateTime() - ChunkStartedAt >= MaxChunkDuration);
 
-                // LastCustomerNo exists only in memory, so a retry cannot exclude
+        exit(true);
-                // work that was already committed.
-                LastCustomerNo := Customer."No.";
-                ProcessedCount += 1;
-
-                // This still opened a FindSet over the complete remaining tail;
-                // periodic commits do not turn retrieval into bounded TOP X.
-                if ProcessedCount mod 500 = 0 then
-                    Commit();
-            until Customer.Next() = 0;
     end;
 }

microsoft/knowledge/performance/avoid-commit-inside-loops.good.al

@@ -14,33 +14,35 @@ query 50127 "Perf Customer Chunk"
 
 codeunit 50128 "Perf Sample CommitInLoop Good"
 {
-    procedure NormalizeCustomerNames()
+    procedure IncreaseCustomerCreditLimits()
     var
-        NormalizeState: Record "Perf Normalize State";
+        CreditLimitState: Record "Perf Credit Limit State";
         LastCustomerNo: Code[20];
     begin
-        if not NormalizeState.Get('CUSTOMER') then begin
+        if not CreditLimitState.Get('CUSTOMER') then begin
-            NormalizeState.Init();
+            CreditLimitState.Init();
-            NormalizeState.Code := 'CUSTOMER';
+            CreditLimitState.Code := 'CUSTOMER';
-            NormalizeState.Insert();
+            CreditLimitState.Insert();
         end;
-        LastCustomerNo := NormalizeState."Last Customer No.";
+        LastCustomerNo := CreditLimitState."Last Customer No.";
 
-        while NormalizeNextChunk(LastCustomerNo) do begin
+        while IncreaseNextChunk(LastCustomerNo) do begin
-            // Persist progress in the same transaction as the completed chunk.
+            CreditLimitState."Last Customer No." := LastCustomerNo;
-            NormalizeState."Last Customer No." := LastCustomerNo;
+            CreditLimitState.Modify();
-            NormalizeState.Modify();
             Commit();
         end;
     end;
 
-    local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean
+    local procedure IncreaseNextChunk(var LastCustomerNo: Code[20]): Boolean
     var
         Customer: Record Customer;
         TempCustomer: Record Customer temporary;
         CustomerChunk: Query "Perf Customer Chunk";
-        LastChunkCustomerNo: Code[20];
+        ChunkStartedAt: DateTime;
+        MaxChunkDuration: Duration;
     begin
+        ChunkStartedAt := CurrentDateTime();
+        MaxChunkDuration := 60000;
         CustomerChunk.TopNumberOfRows(500);
         if LastCustomerNo <> '' then
             CustomerChunk.SetFilter(CustomerNo, '>%1', LastCustomerNo);
@@ -49,7 +51,6 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
             TempCustomer.Init();
             TempCustomer."No." := CustomerChunk.CustomerNo;
             TempCustomer.Insert();
-            LastChunkCustomerNo := CustomerChunk.CustomerNo;
         end;
         CustomerChunk.Close();
 
@@ -60,17 +61,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
         if TempCustomer.FindSet() then
             repeat
                 if Customer.Get(TempCustomer."No.") then begin
-                    Customer.Name := UpperCase(Customer.Name);
+                    Customer."Credit Limit (LCY)" += 100;
                     Customer.Modify();
                 end;
-            until TempCustomer.Next() = 0;
+                LastCustomerNo := TempCustomer."No.";
+            until (TempCustomer.Next() = 0) or (CurrentDateTime() - ChunkStartedAt >= MaxChunkDuration);
 
-        LastCustomerNo := LastChunkCustomerNo;
         exit(true);
     end;
 }
 
-table 50128 "Perf Normalize State"
+table 50128 "Perf Credit Limit State"
 {
     fields
     {

microsoft/knowledge/performance/avoid-commit-inside-loops.md

@@ -1,30 +1,36 @@
 ---
 bc-version: [all]
 domain: performance
-keywords: [commit, commit-in-loop, per-row-commit, checkpoint, bounded-checkpoint, watermark, topnumberofrows]
+keywords: [commit, commit-in-loop, checkpoint, watermark, retry, idempotent, elapsed-time, batch, topnumberofrows]
 technologies: [al]
 countries: [w1]
 application-area: [all]
 ---
 
-# Do not Commit inside loops
+# Commit batches at restart-safe business boundaries
 
 > Contributions welcome — open a PR to refine or extend this article.
 
 ## Description
 
-Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`).
+[Commit ends the current write transaction](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/database/database-commit-method). Committing every row can add transaction overhead and prevent whole-operation rollback, but deliberate commits after N completed business units or an elapsed-time threshold can be valid for long-running work. Most loops need no explicit Commit at all; see [implicit transaction boundaries](understand-implicit-transaction-boundary.md).
 
-A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next).
+Restart safety concerns business effects, not whether a retry revisits a row. A durable checkpoint or processed state can exclude completed work, while demonstrably idempotent replay or durable deduplication can make revisiting it safe. For example, repeating a pure uppercase-name assignment wastes work but does not by itself demonstrate data corruption; repeating an increment can apply it twice.
 
 ## Best Practice
 
-If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
+Choose the commit cadence separately from the retry strategy. Check the row count or elapsed time only after a complete business unit, and commit any final partial batch. A time threshold checked between units is not a fixed-duration guarantee: retrieval, locking, or a single slow unit can exceed it. Let errors propagate so uncommitted work rolls back.
+
+When correctness depends on excluding completed work, persist its checkpoint or processed state in the same transaction as the corresponding business changes. Resume from that committed state, never from a key merely selected for future processing. Alternatively, establish that replay is idempotent or deduplicated for all effects, including external effects; a missing watermark alone is not a correctness finding.
+
+Bounded retrieval is a separate performance requirement. Periodic commits do not cap a full-tail `FindSet`, because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next). When a bounded next-N batch is needed, a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) can fill a temporary key buffer; process only those keys, not an inclusive range that concurrent inserts could expand. Use a stable ordering key and define how to handle records inserted at or below a committed watermark. Do not require bounded retrieval solely because a loop commits.
+
+The paired samples apply a one-time credit-limit increase with one worker over a stable customer set. Both select at most 500 exact keys and finish a chunk after processing them or reaching a one-minute elapsed-time threshold, whichever is observed first. The good sample commits the last processed key with the increases; the bad sample keeps that key only in memory, so a retry can increase already committed limits again. AL DateTime subtraction produces a [Duration in milliseconds](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/duration/duration-data-type).
 
 See sample: [`avoid-commit-inside-loops.good.al`](avoid-commit-inside-loops.good.al).
 
 ## Anti Pattern
 
-Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
+Committing an incomplete business unit, persisting a checkpoint ahead of its business changes, or replaying committed non-idempotent effects with only an in-memory progress variable and no deduplication. In the last case, identify the effect a retry duplicates rather than treating all repeated work as corruption. Committing every row without a reason can also waste transaction overhead; this is distinct from deliberate row-count or elapsed-time batching.
 
 See sample: [`avoid-commit-inside-loops.bad.al`](avoid-commit-inside-loops.bad.al).

microsoft/knowledge/testing/ui-handlers-in-tests.bad.al

@@ -13,7 +13,6 @@ codeunit 50401 "Test UI Handler Proof Bad"
 
         Page.RunModal(Page::"Customer Card", Customer);
 
-        // This only proves a value assigned before the action stayed true.
         Assert.IsTrue(ActionSucceeded, 'The customer card action failed.');
     end;
 
@@ -51,6 +50,32 @@ codeunit 50401 "Test UI Handler Proof Bad"
         Page.RunModal(Page::"Customer Card", Customer);
     end;
 
+    [Test]
+    [HandlerFunctions('ConfirmHandler,PostMessageHandler')]
+    procedure ConfirmPostingAndShowMessage()
+    begin
+        Assert.IsTrue(RunPostingThatConfirmsAndMessages(), 'The posting confirmation was declined.');
+    end;
+
+    local procedure RunPostingThatConfirmsAndMessages(): Boolean
+    begin
+        if not Confirm('Post this document?', false) then
+            exit(false);
+        Message('Posting completed.');
+        exit(true);
+    end;
+
+    [ConfirmHandler]
+    procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean)
+    begin
+        Reply := true;
+    end;
+
+    [MessageHandler]
+    procedure PostMessageHandler(MessageText: Text[1024])
+    begin
+    end;
+
     [ModalPageHandler]
     procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card")
     begin

microsoft/knowledge/testing/ui-handlers-in-tests.good.al

@@ -30,6 +30,41 @@ codeunit 50400 "Test UI Handler Capture Good"
         Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.');
     end;
 
+    [Test]
+    [HandlerFunctions('ConfirmHandler,PostMessageHandler')]
+    procedure ConfirmPostingAndShowMessage()
+    begin
+        LibraryVariableStorage.Clear();
+        LibraryVariableStorage.Enqueue('Post this document?');
+        LibraryVariableStorage.Enqueue(true);
+        LibraryVariableStorage.Enqueue('Posting completed.');
+
+        Assert.IsTrue(RunPostingThatConfirmsAndMessages(), 'The posting confirmation was declined.');
+
+        LibraryVariableStorage.AssertEmpty();
+    end;
+
+    local procedure RunPostingThatConfirmsAndMessages(): Boolean
+    begin
+        if not Confirm('Post this document?', false) then
+            exit(false);
+        Message('Posting completed.');
+        exit(true);
+    end;
+
+    [ConfirmHandler]
+    procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean)
+    begin
+        Assert.ExpectedConfirm(LibraryVariableStorage.DequeueText(), Question);
+        Reply := LibraryVariableStorage.DequeueBoolean();
+    end;
+
+    [MessageHandler]
+    procedure PostMessageHandler(MessageText: Text[1024])
+    begin
+        Assert.ExpectedMessage(LibraryVariableStorage.DequeueText(), MessageText);
+    end;
+
     [ModalPageHandler]
     procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card")
     begin
@@ -45,5 +80,6 @@ codeunit 50400 "Test UI Handler Capture Good"
     var
         Assert: Codeunit "Library Assert";
         LibrarySales: Codeunit "Library - Sales";
+        LibraryVariableStorage: Codeunit "Library - Variable Storage";
         CapturedCustomerNo: Code[20];
 }

microsoft/knowledge/testing/ui-handlers-in-tests.md

@@ -19,12 +19,16 @@ Beyond that wiring guarantee, the test must verify the behavior it cares about.
 
 ## Best Practice
 
-List the handlers the scenario triggers, keep an optional notification handler listed for a notification the scenario may conditionally raise, and make each executed handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`.
+List the handlers the scenario triggers and keep an optional notification handler listed for a notification the scenario may conditionally raise. Make the test and its handlers prove the scenario's contract. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. This proves the captured result, not an exact call count or sequence: repeated calls can overwrite earlier captures.
+
+When interaction count, text, order, or replies are part of the contract, drive the handlers with the `Library - Variable Storage` codeunit. Clear `LibraryVariableStorage` during test initialization, then enqueue expected text and replies in interaction order. A shared `ConfirmHandler` dequeues the expected question, verifies it with `Assert.ExpectedConfirm`, and dequeues the Boolean reply to return; a message handler can use `Assert.ExpectedMessage` with the next expected text. These asserts match a stable text fragment. Finish with `LibraryVariableStorage.AssertEmpty` to catch unconsumed expectations; each handler must also dequeue and verify its interaction so unexpected calls cannot pass silently. Equivalent explicit assertions are valid; absence of this library alone is not a finding.
+
+Prefer one reusable handler of each type within a test codeunit where practical, with individual tests supplying their expectations and replies. This is a maintainability recommendation, not a platform requirement; specialized handlers remain valid when their contracts differ. The paired confirmation-and-message samples run the same flow and check its Boolean result, but only the good sample verifies the expected interactions. The modal capture and optional notification scenarios illustrate contracts that do not require a scripted queue.
 
 See sample: [`ui-handlers-in-tests.good.al`](ui-handlers-in-tests.good.al).
 
 ## Anti Pattern
 
-Omitting a handler for a UI call, listing a nonoptional handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Do not flag the absence of queue storage by itself; require it only when the test needs to prove interaction order, count, text, replies, or a scripted sequence. Do not flag a listed `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` that the run does not reach, and never propose removing one: the entry is what keeps the test passing on the runs where the notification does fire.
+Omitting a handler for a UI call, listing a nonoptional handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Blindly confirming or dismissing messages does not verify a contract that requires specific interactions. Do not flag the absence of queue storage by itself; require interaction verification only when the test needs to prove order, count, text, replies, or a scripted sequence. Do not flag a listed `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` that the run does not reach, and never propose removing one: the entry is what keeps the test passing on the runs where the notification does fire.
 
 See sample: [`ui-handlers-in-tests.bad.al`](ui-handlers-in-tests.bad.al).