diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index c8c9d8d..5f0e50a 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -13,16 +13,26 @@ }, "data-modeling": { "articles": [ - "check-blocked-in-referencing-code-not-in-master", - "extend-report-selection-usage-for-new-document-types", - "document-print-and-email-actions-call-report-selections-directly", - "custom-document-dispatch-must-not-bypass-report-selections", - "transferfields-mirrored-fields-must-match-type-and-length", - "extend-find-entries-navigate-for-new-document-types", "activate-new-price-calculation-handler-via-onfindsupportedsetup", + "check-blocked-in-referencing-code-not-in-master", + "code-must-not-change-workdate", + "custom-document-dispatch-must-not-bypass-report-selections", + "document-print-and-email-actions-call-report-selections-directly", + "extend-find-entries-navigate-for-new-document-types", "extend-price-source-type-must-sync-document-subset-enum", + "extend-report-selection-usage-for-new-document-types", "new-price-source-must-add-candidate-and-trigger-recalculation", - "report-barcodes-must-use-barcode-module-and-production-font-name" + "pictures-must-use-media-not-blob", + "report-barcodes-must-use-barcode-module-and-production-font-name", + "table-design-must-match-bc-table-type-conventions", + "transferfields-mirrored-fields-must-match-type-and-length" + ] + }, + "error-handling": { + "articles": [ + "collect-validation-errors-with-errorbehavior", + "defensive-vs-offensive-code-must-match-blast-radius", + "log-writes-must-survive-rollback" ] }, "events": { @@ -53,7 +63,24 @@ "job-queue-handlers-must-not-require-ui", "job-queue-handlers-must-propagate-failures", "job-queue-on-hold-does-not-stop-running-work", - "store-scheduled-task-id-to-avoid-duplicate-tasks" + "store-scheduled-task-id-to-avoid-duplicate-tasks", + "design-covering-keys-from-read-pattern", + "review-overlapping-keys-before-adding-an-index", + "setcurrentkey-sets-sort-order-not-index-hint", + "preserve-buffered-inserts-by-separating-target-reads", + "aggregate-before-persisting-intermediate-results", + "cache-repeated-filtered-results-with-explicit-scope", + "avoid-repeating-unchanged-validation", + "avoid-get-inside-loop-on-large-table", + "isempty-before-findset-is-extra-round-trip", + "query-results-bypass-primary-key-cache", + "calcsums-instead-of-calcfields-in-loop", + "use-setautocalcfields-for-per-row-flowfields", + "temporary-tables-have-no-database-cost", + "use-setloadfields-for-partial-records", + "prefer-modifyall-over-per-row-modify", + "al-methods-limited-during-write-transactions", + "avoid-user-prompts-inside-transactions" ] }, "privacy": { @@ -97,13 +124,16 @@ "security": { "articles": [ "al-has-no-built-in-htmlencode", - "do-not-concatenate-external-text-into-setfilter" + "do-not-concatenate-external-text-into-setfilter", + "exposed-objects-must-be-in-a-permission-set" ] }, "style": { "articles": [ "label-comment-explains-placeholders", - "dateformula-evaluate-needs-language-independent-literals" + "dateformula-evaluate-needs-language-independent-literals", + "al-comments-must-not-restate-what-code-already-shows", + "pages-must-not-contain-business-logic" ] }, "telemetry": { @@ -112,11 +142,30 @@ "testing": { "articles": [ "ui-handlers-in-tests", - "reset-per-test-state-before-the-isinitialized-guard" + "reset-per-test-state-before-the-isinitialized-guard", + "asserterror-needs-expectederror-and-code", + "bcpt-scenarios-must-be-app-specific", + "commit-shared-test-fixture-inside-lazy-initialize", + "given-blocks-must-cover-full-precondition-chain", + "table-relation-test-exclude-known-invalid-relations-via-event", + "test-feature-scenario-tags", + "test-one-when-per-test", + "transactionmodel-attribute-governs-test-transactions", + "ui-test-codeunit-naming", + "use-assert-isfalse-not-asserterror-for-boolean-checks" + ] + }, + "ui": { + "articles": [ + "default-descending-sort-on-historical-pages", + "page-design-must-match-bc-page-type-conventions" ] }, "upgrade": { - "article": "initvalue-does-not-update-existing-rows", + "articles": [ + "initvalue-does-not-update-existing-rows", + "upgrade-tag-logic-must-not-nest-deeply" + ], "context": "The extended table existed in the previous app version and already contains rows." }, "web-services": { @@ -125,7 +174,8 @@ "handle-httpclient-platform-failure-before-response-access", "check-http-status-before-consuming-response-body", "check-json-null-before-converting-values", - "format-exchanged-values-with-standard-format-9" + "format-exchanged-values-with-standard-format-9", + "api-page-least-privilege-write-access" ] } } diff --git a/microsoft/knowledge/appsource/release-must-update-app-version.md b/microsoft/knowledge/appsource/release-must-update-app-version.md new file mode 100644 index 0000000..fb8f483 --- /dev/null +++ b/microsoft/knowledge/appsource/release-must-update-app-version.md @@ -0,0 +1,37 @@ +--- +bc-version: [all] +domain: appsource +keywords: [version, release, app-json, semver, al-go, appsource] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Update the app version at every release + +## Description + +At every release — a branch merged to `main`, a tagged release build, or an AppSource submission — the app's version is consciously updated, not left to the pipeline alone. + +| Version part | Owner | When | +|---|---|---| +| Major | Developer decision | Breaking change (schema, API, removed objects) | +| Minor | Developer decision | Every release with new functionality | +| Build / Revision | AL-Go pipeline | Automatic — never hand-edited | + +The app's stable identity is its `id` in `app.json`; the version identifies which release — which code state — of that app is deployed. Two customer environments running "the same" version with different code is an undiagnosable support case. AppSource's actual requirement is strict full-version ordering — the complete version must be greater than the previously submitted version — which an AL-Go-generated build/revision increment can satisfy on its own; AppSource does not require major.minor itself to change. Treating major.minor as a deliberate, human-decided compatibility signal is still valuable practice — it is a statement about what changed that no pipeline can make on its own — just not a platform-enforced requirement. + +## Best Practice + + Before the release merge: + app.json: "version": "1.3.0.0" (new functionality -> minor bump, by team convention) + AL-Go settings: "repoVersion": "1.3" (where used) + Then: feature branch -> main via PR, tag, release. + +"Feature branches never touch the version" and "every merge to main is a release" are workflow choices your team can adopt for compatibility clarity — not something AppSource itself requires. + +## Anti Pattern + + Branch merged to main and released. + app.json still says "version": "1.2.0.0" -- same as the previous release. + Two different code states now share one version identity. diff --git a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al new file mode 100644 index 0000000..0c265e9 --- /dev/null +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al @@ -0,0 +1,9 @@ +codeunit 50101 "Batch Job Runner" +{ + procedure AdvanceToNextBusinessDay() + begin + // Anti-pattern: repurposes the user's session WorkDate as a + // scratch variable for unrelated business logic. + WorkDate(CalcDate('<1D>', WorkDate())); + end; +} diff --git a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al new file mode 100644 index 0000000..f1dcf5e --- /dev/null +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al @@ -0,0 +1,11 @@ +codeunit 50100 "Posting Date Helper" +{ + procedure GetDefaultPostingDate(): Date + var + PostingDate: Date; + begin + // Read the work date to default a value; never write to it. + PostingDate := WorkDate(); + exit(PostingDate); + end; +} diff --git a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md new file mode 100644 index 0000000..3e2ea0e --- /dev/null +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md @@ -0,0 +1,58 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [workdate, session-setting, user-control, side-effect] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Application code must not change the WorkDate + +## Description + +The work date is a per-user session setting the user controls from the +client (the date shown in the top-right corner, used to default posting +dates and date filters). Business logic unrelated to that setting must not +call `WorkDate(NewDate)` as a side effect of doing something else — that +silently changes what the user sees and defaults to for the rest of their +session, a surprising, hard-to-trace behavior change the user never asked +for and has no visibility into. This is not a blanket ban on the setter +itself: BCApps' own demo-data generators legitimately save the current +work date, set a specific one to backdate the data they create, and +restore it afterward (see `CreateDemoEDocsBE.Codeunit.al`'s +`WorkDate(SampleInvoiceDate)` / `WorkDate(SavedWorkDate)` pair), and test +codeunits routinely set `WorkDate` deliberately to control the date context +a test runs under (hundreds of calls across BCApps' test suite, for +example `SustainabilityPostingTest.Codeunit.al`). Both are the code's +*actual purpose*, not a side effect of something unrelated. + +This is a call-direction distinction for the read side: reading the +current work date via `WorkDate` (or `WorkDate()` with no argument) is +always fine. + +## Best Practice + +Read the work date to default a value. Only write to it when changing it +*is* the operation being performed — implementing the user's own +work-date/settings action, or a test or demo-data routine that deliberately +establishes a date context (saving and restoring the prior value if the +routine must leave the session as it found it). Business logic that exists +to do something else must never write `WorkDate` as an incidental side +effect; if a calculation needs a specific date, pass or compute that date +as a local variable instead. + +See sample: [`code-must-not-change-workdate.good.al`](code-must-not-change-workdate.good.al). + +## Anti Pattern + +Setting the work date from within a codeunit, report, or page action whose +purpose is unrelated to the user's date preference — for example, a +posting or calculation routine that calls `WorkDate(SomeDate)` to make its +own logic simpler. This changes session state the user owns for the +duration of a call that was never about the work date, and never restores +it. This is a different case from a test or demo-data routine explicitly +declaring a date context: the anti-pattern is unrelated logic silently +mutating state it does not own, not the setter form itself. + +See sample: [`code-must-not-change-workdate.bad.al`](code-must-not-change-workdate.bad.al). diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al new file mode 100644 index 0000000..e20775a --- /dev/null +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al @@ -0,0 +1,11 @@ +table 50111 "Sample Item Card" +{ + fields + { + field(1; "No."; Code[20]) { } + field(50; Picture; BLOB) + { + Caption = 'Picture'; + } + } +} diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al new file mode 100644 index 0000000..4fec633 --- /dev/null +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al @@ -0,0 +1,11 @@ +table 50110 "Sample Item Card" +{ + fields + { + field(1; "No."; Code[20]) { } + field(50; Picture; Media) + { + Caption = 'Picture'; + } + } +} diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md new file mode 100644 index 0000000..1767a0d --- /dev/null +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md @@ -0,0 +1,49 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [blob, media, mediaset, picture-field, image-field, table-design] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Pictures must be stored in a Media/MediaSet field, not BLOB + +## Description + +`BLOB` is still a valid AL field type for arbitrary binary data, but it is +not the right choice for storing pictures or images. The current +recommendation is the `Media` field type for a single image, or +`MediaSet` when a record needs several independent images (e.g. multiple +product photos) — `MediaSet` is a collection of separately-imported media +objects, each with its own identity; it does not generate resized variants +or thumbnails on its own, and displaying more than one item still requires +custom page handling. Media/MediaSet integrate with the platform's +picture control and media repository, which a plain `BLOB` field does not +— but any derived preview or thumbnail image still has to be generated +explicitly and stored in its own field, regardless of which type holds the +source image. + +`BLOB` remains the correct choice for genuinely arbitrary binary payloads +that are not images and don't benefit from the media pipeline (e.g. a raw +file attachment blob unrelated to picture rendering). + +## Best Practice + +Use `Media` for a single image, or `MediaSet` for multiple independent +images, for any field that holds a picture. + +See sample: [`pictures-must-use-media-not-blob.good.al`](pictures-must-use-media-not-blob.good.al). + +## Anti Pattern + +A `BLOB` field named "Picture" compiles and stores the image bytes, but +it misses the picture control integration and media repository that a +`Media`/`MediaSet` field provides for free — the anti pattern is choosing +`BLOB` for image storage out of habit rather than recognizing that the +field is holding a picture, not generic binary data. A related anti +pattern: assuming `MediaSet` gives automatic image variants or thumbnails +because it sounds like a collection with derived versions — it is only a +collection of independently-imported media objects. + +See sample: [`pictures-must-use-media-not-blob.bad.al`](pictures-must-use-media-not-blob.bad.al). diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al new file mode 100644 index 0000000..c241640 --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al @@ -0,0 +1,17 @@ +table 50121 "Sample Ledger Entry" +{ + fields + { + // Anti-pattern: a Ledger table's key must never be user-editable. + field(1; "Entry No."; Integer) { } + field(2; "Posting Date"; Date) { } + field(3; Amount; Decimal) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } + // No AutoIncrement, no guard against manual insert/delete — a user or + // integration can renumber or remove entries, breaking the Ledger + // type's audit-trail guarantee. +} diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al new file mode 100644 index 0000000..825ca45 --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al @@ -0,0 +1,16 @@ +table 50120 "Sample Ledger Entry" +{ + fields + { + // Ledger primary key: Integer "Entry No.", set only by posting. + field(1; "Entry No."; Integer) { AutoIncrement = true; } + field(2; "Posting Date"; Date) { } + field(3; Amount; Decimal) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } + // No user-facing Insert/Delete/Modify path is exposed; rows are + // created exclusively by the posting routine. +} diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md new file mode 100644 index 0000000..ce82f9b --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md @@ -0,0 +1,99 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [tables, table-design, naming-conventions, primary-key, master-table, ledger-table, journal-table, register-table, document-table, setup-table, subsidiary-table, supplemental-table] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Tables must match one of Business Central's nine table-type conventions + +## Description + +Business Central's Base Application follows nine recurring table types — +Master, Supplemental, Subsidiary, Ledger, Register, Journal, Document, +Document History, and Setup. Each type fixes a naming pattern, a +primary-key shape, and a set of associated pages. A new or extended table +whose design doesn't match the conventions of its own type is either +misclassified or built inconsistently with the rest of the application, +and should be flagged in review even if it compiles. Before assigning a +primary key or naming a new table, first identify which of the nine types +it is — that answer fixes almost every other design decision. + +## Best Practice + +Match the table's design to its type: + +1. **Master** (Customer, Item) — one record is the subject; primary key + `Code[20]` named `No.`; description field in `DataCaptionFields`; Card + + List (+ Statistics) pages. +2. **Supplemental** (Currency, Language) — used across functional areas; + primary key `Code[10]` named `Code`; one List page, plural name, set as + `LookupPageID`. +3. **Subsidiary** (Item Vendor) — subsidiary to a Master/Supplemental + table; primary key is the parent key field(s), optionally + `Line No.`; + page shape depends on whether the table carries its own identity: a + pure parent-join table (Item Vendor) typically gets a plain List page + filtered by the calling page, while a subsidiary table that supplements + a master record with its own identity — parent key + own code, e.g. + Ship-to Address, Customer/Vendor Bank Account — commonly gets a + List+Card pair instead, for direct editing of that record. +4. **Ledger** (Cust. Ledger Entry) — transactional record of a functional + area; primary key `Integer` `Entry No.`, always auto-generated by + posting, never user-editable, no free add/delete; List page as + `LookupPageID`/`DrillDownPageID`. +5. **Register** (G/L Register) — table of contents for its Ledger, one row + per posting run; primary key `Integer` `No.`, auto-generated; carries + `From Entry No.`/`To Entry No.`; List page with a link to the Ledger. +6. **Journal** (Resource Journal Line) — where users enter data before + posting to a Ledger; primary key Template + Batch + `Integer` `Line No.`; + Worksheet page with `AutoSplitKey`, a Posting action, and a link to the + Ledger. +7. **Document** (Sales Header/Line) — posts to Ledgers via Journals, not + directly; Header primary key `Code[20]` `No.` (or + `Option Document + Type`); Line primary key = Header key renamed ` No.` + + `Integer Line No.`; Document/Card page with a Posting action and a lines + subpage. +8. **Document History** (Posted Sales Invoice Header/Line) — posted copy of + a Document table, created during posting; mirrors the source table's + fields; never user-editable; same page shape but the Line-equivalent is + a List page, not a Worksheet. +9. **Setup** (General Ledger Setup) — exactly one record for a functional + area; primary key `Code[10]` named `Primary Key`, always blank; one page + with the key field hidden, whose `OnOpenPage` creates the singleton the + first time it's opened (`Reset()` → `Get()` → if not found, `Init()` → + `Insert()`) rather than assuming the record pre-exists. + +A table named "Setup" that holds more than one record follows Subsidiary +rules instead — the name alone is not proof of type. When a table's +identity can't be resolved from its definition alone (e.g. a "Setup"-named +table with a real business-field key and no page), say so explicitly +rather than forcing a classification; settling it requires checking actual +row cardinality or call sites, not just the object definition. + +These nine types cover Business Central's *business-record* tables — they +are not an exhaustive catalogue of every legitimate table shape. A +temporary/buffer table, a work queue, a log or telemetry table, a +cross-reference/mapping table with no business meaning of its own, or a +staging/working table used only inside one process is not required to fit +any of the nine, and forcing one into the nearest-looking type (usually +Ledger, because it has an `Integer` key, or Subsidiary, because it has a +composite key) produces a harmful redesign recommendation for a table that +was never meant to carry that type's guarantees. Apply this rule only when +the table's name, fields, or usage genuinely establish it as one of the +nine business-record types; when nothing points that way, this rule simply +does not apply — that is not the same as an unresolved classification. + +See sample: [`table-design-must-match-bc-table-type-conventions.good.al`](table-design-must-match-bc-table-type-conventions.good.al). + +## Anti Pattern + +A table that mixes conventions from two types — for example, a "Ledger" +table with a user-editable primary key that lets users freely insert or +delete rows — is not "flexible", it is either misclassified or has skipped +a design step. A Ledger table's `Entry No.` must come only from the +posting routine; exposing it as an editable field breaks the type's core +guarantee that entries are an immutable, sequential audit trail. + +See sample: [`table-design-must-match-bc-table-type-conventions.bad.al`](table-design-must-match-bc-table-type-conventions.bad.al). diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al new file mode 100644 index 0000000..6f2835f --- /dev/null +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al @@ -0,0 +1,11 @@ +// Both fields guarded the same way, out of habit rather than analysis. +if Customer.Get(SalesHeader."Sell-to Customer No.") then + CustomerHomePage := Customer."Home Page"; // low blast radius - fine + +// but the same pattern, unexamined, was also applied here: +if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then + VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group" +else + VATBusPostingGroup := ''; + // High blast radius: silently wrong VAT posting group reaches posting + // with no error, no TestField, and no reviewer in the loop. diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al new file mode 100644 index 0000000..35f88ee --- /dev/null +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al @@ -0,0 +1,14 @@ +// Low blast radius: guard, with an explicit chosen fallback. +if Customer.Get(SalesHeader."Sell-to Customer No.") then + CustomerHomePage := Customer."Home Page" +else + CustomerHomePage := ''; +// Blank is an acceptable, deliberately-considered default here - the field +// is purely a display convenience and a reviewer sees it before the document ships. +// It is assigned explicitly, though, not left to whatever the variable +// happened to hold before this lookup ran. + +// High blast radius: let it fail loud, because this feeds posted VAT. +SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo); +SalesHeader.TestField("VAT Bus. Posting Group"); +VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group"; diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md new file mode 100644 index 0000000..91a8aff --- /dev/null +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [defensive-programming, offensive-programming, fail-fast, blast-radius, guarded-lookup] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Match defensive vs. offensive error handling to the blast radius of being wrong + +## Description + +Whether code should guard gracefully (defensive) or fail loudly (offensive/fail-fast) is not a matter of habit or a blanket house style — it depends on what happens downstream if the guarded condition is silently defaulted or skipped. Treating every missing value the same way, defensively or offensively, is itself the anti-pattern: uniform defensiveness hides the failures that matter most, while uniform fail-fast turns ordinary, expected absence into unnecessary crashes. Two fields can look structurally identical — both read from a related record, both potentially missing — and still deserve opposite treatment depending on what they feed. + +## Best Practice + +Trace what a silently-defaulted or skipped value actually reaches before deciding how to guard it. If it reaches a posted ledger amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output, code offensively: let the lookup fail loud (`TestField`, an unguarded `Get()` expected to always succeed, or an explicit `Error`) so a human sees the problem before anything posts. If it is cosmetic, informational, or easily corrected after the fact (a display field, an optional UI enhancement, a report not yet run), code defensively — but the fallback must be an explicit, deliberately-chosen, named business value, never a blank or zero that is merely the datatype default. When genuinely unsure which category a field falls into, that is a question to resolve explicitly with whoever owns the requirement, not a coin flip. + +See sample: [`defensive-vs-offensive-code-must-match-blast-radius.good.al`](defensive-vs-offensive-code-must-match-blast-radius.good.al). + +## Anti Pattern + +Guarding two fields the same way purely out of habit, without analyzing what each one feeds. A low-blast-radius field, such as a customer's home page URL shown only for convenience on a printed document, and a high-blast-radius field, such as the VAT posting group that determines VAT actually applied to a posted transaction, are both wrapped in the same `if Header.Get(...) then ... else` pattern with a blank/zero fallback — leaving the posting-critical field free to post with a silently wrong value. A VAT registration number is not a safe stand-in for the low-risk side of this example: it is legally relevant, often validated, and can feed external VAT services or mandated document output, so it belongs on the offensive/fail-fast side alongside the posting group, not next to it as the "safe" contrast. + +See sample: [`defensive-vs-offensive-code-must-match-blast-radius.bad.al`](defensive-vs-offensive-code-must-match-blast-radius.bad.al). diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al new file mode 100644 index 0000000..6cc2876 --- /dev/null +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al @@ -0,0 +1,24 @@ +codeunit 50101 "Sample Web Service Caller" +{ + procedure CallExternalService() + var + ErrorLogEntry: Record "Sample Error Log"; + begin + // BUG: the log write happens inside the same transaction as the + // risky call, using the same Record instance as the caller. + if not TryCallService() then begin + ErrorLogEntry.Init(); + ErrorLogEntry."Error Message" := CopyStr(GetLastErrorText(), 1, 250); + ErrorLogEntry.Insert(); + Error(GetLastErrorText()); + // Error() above rolls back this transaction - including the + // Insert() just made. The failure is never actually logged. + end; + end; + + [TryFunction] + local procedure TryCallService() + begin + // ... external call that may fail ... + end; +} diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al new file mode 100644 index 0000000..c0f7d65 --- /dev/null +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al @@ -0,0 +1,45 @@ +table 50100 "Sample Error Log Buffer" +{ + TableType = Temporary; + fields + { + field(1; "Call Duration (ms)"; Integer) { } + field(2; "Error Message"; Text[250]) { } + } +} + +codeunit 50100 "Sample Error Log Writer" +{ + // TableNo makes OnRun receive the Record that Session.StartSession + // passes to the new session. This is the only data channel into that + // session — there is no shared memory with the caller's instance. + TableNo = "Sample Error Log Buffer"; + + trigger OnRun() + var + ErrorLogEntry: Record "Sample Error Log"; + begin + ErrorLogEntry.Init(); + ErrorLogEntry."Call Duration (ms)" := Rec."Call Duration (ms)"; + ErrorLogEntry."Error Message" := + CopyStr(Rec."Error Message", 1, MaxStrLen(ErrorLogEntry."Error Message")); + ErrorLogEntry.Insert(true); + Commit(); + end; +} + +// Caller side: populate the buffer record, then hand it to StartSession. +// The insert-and-commit above happens inside the started session, so it +// survives even if the caller's own transaction rolls back afterward. +codeunit 50101 "Sample Error Log Caller Excerpt" +{ + procedure LogFailure(Duration: Integer; ErrorText: Text) + var + LogBuffer: Record "Sample Error Log Buffer" temporary; + SessionId: Integer; + begin + LogBuffer."Call Duration (ms)" := Duration; + LogBuffer."Error Message" := CopyStr(ErrorText, 1, MaxStrLen(LogBuffer."Error Message")); + Session.StartSession(SessionId, Codeunit::"Sample Error Log Writer", CompanyName, LogBuffer); + end; +} diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md new file mode 100644 index 0000000..eae1700 --- /dev/null +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [logging, rollback, session, transaction, isolated-session, telemetry] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Log writes that must capture failures must survive transaction rollback + +## Description + +Inserting a log record inside the same transaction as the operation it logs looks correct until the operation errors: the transaction rolls back and takes the log entry with it. The result is a log that faithfully records every success and silently loses exactly the failures it exists to capture. This is a common blind spot in error/duration logging around web-service calls, background jobs, and other operations expected to fail sometimes. + +## Best Practice + +Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`Session.StartSession` on a `TableNo`-scoped codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. `StartSession`'s only channel for getting data into that new session is its optional `Record` parameter, delivered to the target codeunit's `OnRun` trigger — a separate session gets a fresh instantiation of the codeunit, so calling a setter procedure on a local object variable before starting the session does not populate anything in the new session's instance. Capture duration and other telemetry values in the caller, place them into the `Record` passed to `StartSession`, and do the insert-and-commit entirely inside that session's own `OnRun`. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically. + +See sample: [`log-writes-must-survive-rollback.good.al`](log-writes-must-survive-rollback.good.al). + +## Anti Pattern + +Inserting the error-log record in the same transaction as the risky operation, so a rollback deletes the very entry meant to explain the failure. Adding a stray `Commit` before the risky call is not a fix either — it breaks the caller's atomicity and can violate posting-routine rules. Swallowing the error to keep the log alive (running a codeunit without checking or re-raising its result) is equally wrong: the log must observe the failure, not suppress it. + +See sample: [`log-writes-must-survive-rollback.bad.al`](log-writes-must-survive-rollback.bad.al). diff --git a/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.bad.al b/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.bad.al new file mode 100644 index 0000000..9e3de05 --- /dev/null +++ b/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.bad.al @@ -0,0 +1,73 @@ +// Input stays stable for this run; output contains one row per group with entries. +table 50367 "Perf Posted Entry" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; "Location Code"; Code[10]) { } + field(4; "Posting Date"; Date) { } + field(5; Quantity; Decimal) { } + } + + keys + { + key(PK; "Entry No.") { Clustered = true; } + key(ByItemLocationDate; "Item No.", "Location Code", "Posting Date") + { + SumIndexFields = Quantity; + } + } +} + +table 50368 "Perf Quantity Summary" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Run ID"; Guid) { } + field(2; "Item No."; Code[20]) { } + field(3; "Location Code"; Code[10]) { } + field(4; Quantity; Decimal) { } + } + + keys + { + key(PK; "Run ID", "Item No.", "Location Code") { Clustered = true; } + } +} + +codeunit 50369 "Perf Summary Bad" +{ + procedure BuildSummary(CutoffDate: Date) RunId: Guid + var + Entry: Record "Perf Posted Entry"; + Scan: Record "Perf Posted Entry"; + Summary: Record "Perf Quantity Summary"; + begin + RunId := CreateGuid(); + Entry.SetFilter("Posting Date", '..%1', CutoffDate); + if Entry.FindSet() then + repeat + Scan.SetRange("Item No.", Entry."Item No."); + Scan.SetRange("Location Code", Entry."Location Code"); + Scan.SetFilter("Posting Date", '..%1', CutoffDate); + Scan.CalcSums(Quantity); + + if Summary.Get(RunId, Entry."Item No.", Entry."Location Code") then begin + Summary.Quantity := Scan.Quantity; + Summary.Modify(false); + end else begin + Summary.Init(); + Summary."Run ID" := RunId; + Summary."Item No." := Entry."Item No."; + Summary."Location Code" := Entry."Location Code"; + Summary.Quantity := Scan.Quantity; + Summary.Insert(false); + end; + until Entry.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.good.al b/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.good.al new file mode 100644 index 0000000..c94e970 --- /dev/null +++ b/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.good.al @@ -0,0 +1,93 @@ +// Input stays stable for this run; output contains one row per group with entries. +table 50367 "Perf Posted Entry" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; "Location Code"; Code[10]) { } + field(4; "Posting Date"; Date) { } + field(5; Quantity; Decimal) { } + } + + keys + { + key(PK; "Entry No.") { Clustered = true; } + key(ByItemLocationDate; "Item No.", "Location Code", "Posting Date") + { + SumIndexFields = Quantity; + } + } +} + +table 50368 "Perf Quantity Summary" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Run ID"; Guid) { } + field(2; "Item No."; Code[20]) { } + field(3; "Location Code"; Code[10]) { } + field(4; Quantity; Decimal) { } + } + + keys + { + key(PK; "Run ID", "Item No.", "Location Code") { Clustered = true; } + } +} + +query 50370 "Perf Grouped Quantity" +{ + QueryType = Normal; + + elements + { + dataitem(Entry; "Perf Posted Entry") + { + column(ItemNo; "Item No.") { } + column(LocationCode; "Location Code") { } + column(TotalQuantity; Quantity) + { + Method = Sum; + } + filter(PostingDate; "Posting Date") { } + } + } +} + +codeunit 50369 "Perf Summary Good" +{ + procedure BuildSummary(CutoffDate: Date) RunId: Guid + var + Totals: Query "Perf Grouped Quantity"; + TempSummary: Record "Perf Quantity Summary" temporary; + Summary: Record "Perf Quantity Summary"; + begin + RunId := CreateGuid(); + Totals.SetFilter(PostingDate, '..%1', CutoffDate); + Totals.Open(); + while Totals.Read() do begin + TempSummary.Init(); + TempSummary."Run ID" := RunId; + TempSummary."Item No." := Totals.ItemNo; + TempSummary."Location Code" := Totals.LocationCode; + TempSummary.Quantity := Totals.TotalQuantity; + TempSummary.Insert(); + end; + Totals.Close(); + + if TempSummary.FindSet() then + repeat + Summary.Init(); + Summary."Run ID" := RunId; + Summary."Item No." := TempSummary."Item No."; + Summary."Location Code" := TempSummary."Location Code"; + Summary.Quantity := TempSummary.Quantity; + Summary.Insert(false); + until TempSummary.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.md b/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.md new file mode 100644 index 0000000..bdff7ca --- /dev/null +++ b/microsoft/knowledge/performance/aggregate-before-persisting-intermediate-results.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: performance +keywords: [query, grouping, aggregate, intermediate-results, temporary-buffer, persistent-writes, calcsums, modify, method-sum] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Aggregate source rows before persisting completed results + +## Description + +A batch may repeatedly calculate the same group total while inserting and updating persistent working rows, even though only one completed result per group is needed. An AL Query with `Method = Sum` groups by its other output columns, so the required grain can often be computed from qualifying source rows before writing final output. This is a data-path change, not a blanket replacement for posting or for intermediate rows needed for recovery or business behavior. + +## Best Practice + +Specify the output grain and whether absent groups should produce zero rows. Apply source filters (including cutoff dates), aggregate at exactly that grain, and write only completed results; a small temporary buffer can separate the query from persistent output, but streaming or bounded units may be better for large grouped sets. Avoid a one-to-many join that duplicates quantities or an extra output column that silently changes the grouping. Preserve security filters, signs, units, FlowFilters, relevant master-data restrictions, transaction/trigger behavior, and acceptable read consistency; a shared date cutoff does not create a snapshot. Compare complete keyed outputs and measure source reads, repeated calculations, temporary memory, and persistent writes. See sample: [`aggregate-before-persisting-intermediate-results.good.al`](aggregate-before-persisting-intermediate-results.good.al). + +## Anti Pattern + +For an output that only needs one signed total per item/location with qualifying entries, summing the same source range and rewriting a persistent summary for *every* entry. Do not flag incremental persisted state that is required for locking, resumability, or downstream processing, or require an in-memory copy of all raw history to perform SQL grouping. See sample: [`aggregate-before-persisting-intermediate-results.bad.al`](aggregate-before-persisting-intermediate-results.bad.al). + +## References + +- [Aggregating data in query objects](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-query-totals-grouping). +- [Query performance](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-query-objects-and-performance). +- [Buffered inserts](preserve-buffered-inserts-by-separating-target-reads.md). diff --git a/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.bad.al b/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.bad.al new file mode 100644 index 0000000..665520b --- /dev/null +++ b/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.bad.al @@ -0,0 +1,18 @@ +codeunit 50258 "Perf Sample RunModalInTxn Bad" +{ + procedure ChangeShippingAgent(DocNo: Code[20]) + var + SalesHeader: Record "Sales Header"; + ShippingAgent: Record "Shipping Agent"; + begin + SalesHeader.Get(SalesHeader."Document Type"::Order, DocNo); + SalesHeader."Shipment Date" := WorkDate(); + SalesHeader.Modify(true); // a write transaction is now open + + // Runtime error: RunModal is not allowed in write transactions. + if Page.RunModal(Page::"Shipping Agents", ShippingAgent) = Action::LookupOK then begin + SalesHeader.Validate("Shipping Agent Code", ShippingAgent.Code); + SalesHeader.Modify(true); + end; + end; +} diff --git a/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.good.al b/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.good.al new file mode 100644 index 0000000..0fcfcb2 --- /dev/null +++ b/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.good.al @@ -0,0 +1,18 @@ +codeunit 50259 "Perf Sample RunModalInTxn Good" +{ + procedure ChangeShippingAgent(DocNo: Code[20]) + var + SalesHeader: Record "Sales Header"; + ShippingAgent: Record "Shipping Agent"; + begin + // User interaction first, while no write transaction is open. + if Page.RunModal(Page::"Shipping Agents", ShippingAgent) <> Action::LookupOK then + exit; + + // All writes after the choice is made; the transaction ends with the trigger. + SalesHeader.Get(SalesHeader."Document Type"::Order, DocNo); + SalesHeader."Shipment Date" := WorkDate(); + SalesHeader.Validate("Shipping Agent Code", ShippingAgent.Code); + SalesHeader.Modify(true); + end; +} diff --git a/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.md b/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.md new file mode 100644 index 0000000..2bbedb3 --- /dev/null +++ b/microsoft/knowledge/performance/al-methods-limited-during-write-transactions.md @@ -0,0 +1,46 @@ +--- +bc-version: [all] +domain: performance +keywords: [limited-during-write-transactions, write-transaction, runmodal, report-run, page-runmodal, report-runmodal, xmlport-run, codeunit-run, commit, requestpage] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL methods limited during write transactions: commit before RunModal and Codeunit.Run + +## Description + +Once AL code has written to the database in the current transaction — an `Insert`, `Modify`, or `Delete` with no `Commit` since — the platform restricts four methods until that transaction is committed. The exact conditions, as enforced: + +- `Page.RunModal` — not allowed in a write transaction, under any circumstances. +- `Report.RunModal` and `Report.Run` — both allowed only if the request page is suppressed: `Report.RunModal(ReportId, false)` / `Report.Run(ReportId, false)` (the second argument is `RequestWindow` in both), or `UseRequestPage(false)` on a report instance. With a request page shown, either fails identically — `Run` and `RunModal` differ only in whether the report instance is cleared afterward, not in this guard. +- `Xmlport.Run` — same rule when it would show a request page: allowed only if the request page is suppressed, `Xmlport.Run(XmlPortId, false)` (the second argument is `RequestWindow`) or the `UseRequestPage = false;` object property. With a request page shown it fails. There is no `XmlPort.RunModal` method — see the note on the platform's error text below. +- `Codeunit.Run` — allowed only if its Boolean return value is not used. `OK := Codeunit.Run()` and `if Codeunit.Run() then` fail, because that form commits — see `codeunit-run-requires-prior-commit-inside-transaction.md`. + +This is a runtime error, not a compiler diagnostic: the code builds, and the first execution that reaches the call with an open write transaction dies. The guard keys on transaction state alone, not on any relation between what was written and what is opened: an `Item.Insert()` followed by `Page.RunModal(Page::"Customer Card")` — an unrelated table — fails on the `RunModal` line, and because the error stops the transaction, the insert rolls back with it. + +The platform's message (Business Central 26, reproduced 2026-09-07) reads: "The following AL methods are limited during write transactions because one or more tables will be locked: Form.RunModal, Codeunit.Run, Report.RunModal, XmlPort.RunModal. Form.RunModal is not allowed in write transactions. Codeunit.Run is allowed in write transactions only if the return value is not used. For example, 'OK := Codeunit.Run()' is not allowed. Report.RunModal is allowed in write transactions only if 'RequestForm = false'. For example, 'Report.RunModal(...,false)' is allowed. XmlPort.RunModal is allowed in write transactions only if 'RequestForm = false'. For example, 'XmlPort.RunModal(...,false)' is allowed. Use the commit method to save the changes before this call, or structure the code differently." The message still uses the legacy names `Form.RunModal` and `RequestForm` even though the AL method is `Page.RunModal` and the report parameter is `RequestWindow`; older versions said "C/AL functions" instead of "AL methods" (microsoft/AL#5452, 2019). The message also names `XmlPort.RunModal`, which is legacy phrasing too — there is no such AL method; the actual API the guard applies to is `Xmlport.Run`, and the message's `RequestForm = false` condition corresponds to `Xmlport.Run`'s `RequestWindow` argument or the `UseRequestPage` property. The behavior is unchanged across versions. + +The reason is the same one behind `avoid-user-prompts-inside-transactions.md`: a modal object waits for the user, and the platform will not let a write transaction — and every lock it holds — sit open for as long as that takes. The difference is enforcement. `Confirm` and `StrMenu` are *allowed* inside a write transaction and silently hold the locks; `RunModal` is *refused*. Both point at the same design fix. + +## Best Practice + +Sequence the work so the modal interaction happens before the write phase: run the lookup or dialog page first, then perform the writes the user's choice requires, and let the transaction end. When a modal object genuinely must follow a write, `Commit()` first — but only when the state written so far is complete and safe to persist on its own, because that Commit is a real transaction boundary, not a formality. Microsoft's own Base Application follows exactly this pattern where the preceding state is final (`ActivityLog.Table.al` commits the log entry before `Page.RunModal(Page::"Activity Log", Rec)`; `DocumentSendingProfile.Table.al` commits before `Page.RunModal(Page::"Select Sending Options", …)`). For a report, suppressing the request page — `Report.UseRequestPage(false)` on a report instance, or `false` as the `RequestWindow` argument to `Run`/`RunModal` — is a legitimate way to run it inside a write transaction when no user input is needed. For an XMLport, the equivalent is `false` as the `RequestWindow` argument to `Xmlport.Run`, or the `UseRequestPage = false;` object property; XMLports have no instance `UseRequestPage` method. `Database.IsInWriteTransaction()` (runtime 11.0+) lets library code that cannot control its caller detect the state, with the same caveat as the Codeunit.Run article: branching production flow on it usually signals unclear transaction ownership. + +See sample: [`al-methods-limited-during-write-transactions.good.al`](al-methods-limited-during-write-transactions.good.al). + +## Anti Pattern + +Writing to the database and then calling `Page.RunModal` (or a report/XMLport with its request page) in the same trigger — the first production run hits the runtime error. The reflexive fixes are worse than the error: dropping in `Commit()` to silence it persists a half-finished state that can no longer roll back with the rest of the operation, and swapping the page for a `Confirm` or `StrMenu` to "avoid the error" trades a loud failure for the silent lock-holding that `avoid-user-prompts-inside-transactions.md` warns about. + +See sample: [`al-methods-limited-during-write-transactions.bad.al`](al-methods-limited-during-write-transactions.bad.al). + +## Source + +- Microsoft Learn, `Codeunit.Run` transaction semantics ("If you're already in a transaction you must commit first before calling Codeunit.Run"): https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/codeunit/codeunit-run-method +- Microsoft Learn, `Xmlport.Run(Integer [, Boolean] [, Boolean] [, var Record])` (the `RequestWindow` argument; confirms the XMLport data type has no `RunModal` method, static or instance): https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/xmlport/xmlport-run-method +- Microsoft Learn, `UseRequestPage` property ("Applies to: Xml Port, Report" — an object property, not an XMLport instance method; the instance method of the same name exists only on `Report`): https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-userequestpage-property +- microsoft/AL issue #5452 (verbatim English error text, reproduced on "any current version of Business Central", 2019-11-13): https://github.com/microsoft/AL/issues/5452 +- Reproduced 2026-09-07 on Business Central 26 (runtime error dialog, English and Danish clients): a page `OnAction` doing `Item.Insert()` then `Page.RunModal(Page::"Customer Card")` fails with the text quoted in the Description, the AL call stack pointing at the `RunModal` line, and the dialog stating that the transaction was stopped. +- Microsoft Base Application (BCApps, W1): the `Commit(); … Page.RunModal(…)` pattern in `Modules/System/Logging/ActivityLog.Table.al`, `Foundation/Reporting/DocumentSendingProfile.Table.al`, `Bank/Setup/PaymentServiceSetup.Table.al`, and others; BCApps test suites annotate the same guard as "COMMIT is required for Write Transaction Error" (`Tests/General Journal/ERMTestMultipleGenJnlLines.Codeunit.al`). diff --git a/microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md b/microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md index db4f5e1..08c73a9 100644 --- a/microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md +++ b/microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: performance -keywords: [n-plus-one, get, findfirst, loop, inner-lookup, large-table] +keywords: [n-plus-one, get, findfirst, loop, inner-lookup, large-table, item-get, query] technologies: [al] countries: [w1] application-area: [all] @@ -15,7 +15,7 @@ A `Get` or `FindFirst` against another persistent table inside a loop can produc ## Best Practice -Use a query object to join the outer and inner tables when the relationship and filters can be expressed as one query. If keys repeat, a dictionary cache can reduce lookups to one per distinct key. `SetLoadFields` can reduce the columns transferred by unavoidable inner reads, but it does not eliminate the N+1 shape and must not be presented as doing so. +Use a query object to join the outer and inner tables when the relationship and filters can be expressed as one query. If complete lookup keys repeat and the result remains valid, a [scoped cache](cache-repeated-filtered-results-with-explicit-scope.md) can reduce lookups to one per distinct key; do not assume primary-key `Get` calls each hit SQL (see [transaction caching](primary-key-get-in-loop-is-transaction-cached.md)). `SetLoadFields` can reduce the columns transferred by unavoidable inner reads, but it does not eliminate the N+1 shape and must not be presented as doing so. See sample: [`avoid-get-inside-loop-on-large-table.good.al`](avoid-get-inside-loop-on-large-table.good.al). diff --git a/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.bad.al b/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.bad.al new file mode 100644 index 0000000..d3bc03b --- /dev/null +++ b/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.bad.al @@ -0,0 +1,39 @@ +// Quantity validation depends only on Quantity and Unit Price in this demo. +table 50371 "Perf Validated Line" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Line No."; Integer) { } + field(2; Quantity; Decimal) + { + trigger OnValidate() + begin + Amount := Quantity * "Unit Price"; + end; + } + field(3; "Unit Price"; Decimal) { } + field(4; Amount; Decimal) { } + field(5; Note; Text[100]) { } + } + + keys + { + key(PK; "Line No.") { Clustered = true; } + } +} + +codeunit 50372 "Perf Validation Bad" +{ + procedure UpdateLine(LineNo: Integer; NewQuantity: Decimal; NewNote: Text[100]) + var + Line: Record "Perf Validated Line"; + begin + Line.Get(LineNo); + Line.Validate(Quantity, NewQuantity); + Line.Note := NewNote; + Line.Validate(Quantity, NewQuantity); + Line.Modify(false); + end; +} diff --git a/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.good.al b/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.good.al new file mode 100644 index 0000000..de03f3a --- /dev/null +++ b/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.good.al @@ -0,0 +1,38 @@ +// Quantity validation depends only on Quantity and Unit Price in this demo. +table 50371 "Perf Validated Line" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Line No."; Integer) { } + field(2; Quantity; Decimal) + { + trigger OnValidate() + begin + Amount := Quantity * "Unit Price"; + end; + } + field(3; "Unit Price"; Decimal) { } + field(4; Amount; Decimal) { } + field(5; Note; Text[100]) { } + } + + keys + { + key(PK; "Line No.") { Clustered = true; } + } +} + +codeunit 50372 "Perf Validation Good" +{ + procedure UpdateLine(LineNo: Integer; NewQuantity: Decimal; NewNote: Text[100]) + var + Line: Record "Perf Validated Line"; + begin + Line.Get(LineNo); + Line.Validate(Quantity, NewQuantity); + Line.Note := NewNote; + Line.Modify(false); + end; +} diff --git a/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.md b/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.md new file mode 100644 index 0000000..96325b3 --- /dev/null +++ b/microsoft/knowledge/performance/avoid-repeating-unchanged-validation.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: performance +keywords: [validate, onvalidate, repeated-write, sales-line, subscriber, unchanged-value] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Avoid repeating validation when its inputs have not changed + +## Description + +`Validate` runs field validation logic, which can invoke subscribers and additional reads or writes even if the assigned value is unchanged. When a document-building path validates the same field twice without changing any input its validation depends on, the second cascade may do the same work again. A field-value comparison alone does not prove that the dependent context or required event behavior is unchanged. + +## Best Practice + +Trace the table's `OnValidate`, subscribers, and dependent fields before removing a duplicate invocation. Keep the required validation and write, but skip a second `Validate` only when all its inputs, its order-dependent effects, and the business contract are demonstrably unchanged. Measure validations and writes per business unit and test pricing, reservations, error behavior, and subscriber effects on real document paths. The sample's custom field validation depends only on Quantity and Unit Price; a note assigned between calls does not affect either. See sample: [`avoid-repeating-unchanged-validation.good.al`](avoid-repeating-unchanged-validation.good.al). + +## Anti Pattern + +Validating Quantity, changing only an unrelated local or record note, validating the same Quantity again, then modifying the row, without any required second event. Do not replace `Validate` with direct assignment, use `Insert(false)` indiscriminately, or reorder validations on a Sales Line solely to reduce call counts: its standard logic can depend on other fields and event subscribers. See sample: [`avoid-repeating-unchanged-validation.bad.al`](avoid-repeating-unchanged-validation.bad.al). + +## References + +- [Record.Validate and field validation behavior](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-validate-method). +- [Equivalent bulk assignments versus per-row validation](prefer-modifyall-over-per-row-modify.md). +- [Subscriber guards](guard-event-subscribers-before-db-call.md). diff --git a/microsoft/knowledge/performance/avoid-user-prompts-inside-transactions.md b/microsoft/knowledge/performance/avoid-user-prompts-inside-transactions.md index bb03f41..f871c98 100644 --- a/microsoft/knowledge/performance/avoid-user-prompts-inside-transactions.md +++ b/microsoft/knowledge/performance/avoid-user-prompts-inside-transactions.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -A `Confirm`, `StrMenu`, modal page, or other user prompt issued from inside a write transaction stalls the transaction — and therefore every lock it holds — until the user responds. Per the upstream guidance, "Avoid user interactions (Confirm, StrMenu) inside transactions — they hold locks while waiting for user input." The wait is bounded only by the user; meanwhile other sessions block on whatever this transaction has acquired. +A `Confirm`, `StrMenu`, or other user prompt issued from inside a write transaction stalls the transaction — and therefore every lock it holds — until the user responds. Per the upstream guidance, "Avoid user interactions (Confirm, StrMenu) inside transactions — they hold locks while waiting for user input." The wait is bounded only by the user; meanwhile other sessions block on whatever this transaction has acquired. ## Best Practice diff --git a/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.bad.al b/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.bad.al new file mode 100644 index 0000000..e200297 --- /dev/null +++ b/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.bad.al @@ -0,0 +1,24 @@ +codeunit 50363 "Perf Variant Cache Bad" +{ + procedure CountAndCollectVariantLines(var TempSalesLine: Record "Sales Line" temporary; OrderNo: Code[20]; var VariantItemNos: List of [Code[20]]) VariantLines: Integer + var + ItemVariant: Record "Item Variant"; + begin + TempSalesLine.SetRange("Document Type", TempSalesLine."Document Type"::Order); + TempSalesLine.SetRange("Document No.", OrderNo); + TempSalesLine.SetRange(Type, TempSalesLine.Type::Item); + if TempSalesLine.FindSet() then + repeat + ItemVariant.SetRange("Item No.", TempSalesLine."No."); + if not ItemVariant.IsEmpty() then + VariantLines += 1; + until TempSalesLine.Next() = 0; + + if TempSalesLine.FindSet() then + repeat + ItemVariant.SetRange("Item No.", TempSalesLine."No."); + if not ItemVariant.IsEmpty() then + VariantItemNos.Add(TempSalesLine."No."); + until TempSalesLine.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.good.al b/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.good.al new file mode 100644 index 0000000..d5db5d6 --- /dev/null +++ b/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.good.al @@ -0,0 +1,48 @@ +codeunit 50363 "Perf Variant Cache Good" +{ + procedure CountAndCollectVariantLines(var TempSalesLine: Record "Sales Line" temporary; OrderNo: Code[20]; var VariantItemNos: List of [Code[20]]) VariantLines: Integer + var + ItemVariant: Record "Item Variant"; + HasVariantsByItem: Dictionary of [Code[20], Boolean]; + begin + TempSalesLine.SetRange("Document Type", TempSalesLine."Document Type"::Order); + TempSalesLine.SetRange("Document No.", OrderNo); + TempSalesLine.SetRange(Type, TempSalesLine.Type::Item); + if TempSalesLine.FindSet() then + repeat + if HasVariants(TempSalesLine."No.", ItemVariant, HasVariantsByItem) then + VariantLines += 1; + until TempSalesLine.Next() = 0; + + if TempSalesLine.FindSet() then + repeat + if HasVariants(TempSalesLine."No.", ItemVariant, HasVariantsByItem) then + VariantItemNos.Add(TempSalesLine."No."); + until TempSalesLine.Next() = 0; + end; + + local procedure HasVariants(ItemNo: Code[20]; var ItemVariant: Record "Item Variant"; var HasVariantsByItem: Dictionary of [Code[20], Boolean]): Boolean + var + CachedResult: Boolean; + begin + if HasVariantsByItem.Get(ItemNo, CachedResult) then + exit(CachedResult); + + ItemVariant.SetRange("Item No.", ItemNo); + CachedResult := not ItemVariant.IsEmpty(); + HasVariantsByItem.Add(ItemNo, CachedResult); + exit(CachedResult); + end; + + procedure CountDistinctItemsWithVariants(var TempItems: Record Item temporary) VariantItems: Integer + var + ItemVariant: Record "Item Variant"; + begin + if TempItems.FindSet() then + repeat + ItemVariant.SetRange("Item No.", TempItems."No."); + if not ItemVariant.IsEmpty() then + VariantItems += 1; + until TempItems.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.md b/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.md new file mode 100644 index 0000000..b85bff0 --- /dev/null +++ b/microsoft/knowledge/performance/cache-repeated-filtered-results-with-explicit-scope.md @@ -0,0 +1,27 @@ +--- +bc-version: [all] +domain: performance +keywords: [cache, dictionary, filtered-lookup, isempty, repeated-query, invalidation, scope] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Reuse repeated filtered results within a correct cache scope + +## Description + +A loop can ask the same filtered existence or calculation question for many rows sharing a business key, or two phases can ask it for the same rows. Unlike repeated primary-key `Get` calls, non-keyed filtered lookups are not automatically answered by the primary-key record cache. Memoization can remove repeated AL and data-access work, but a cache keyed by too few inputs or kept past a data change returns the wrong answer. A single pass over lines with unknown, possibly distinct item numbers does not establish reuse. + +## Best Practice + +First reuse an already-loaded result if valid. Require evidence that complete lookup keys actually repeat, such as repeated queries for the same item in two passes over an unchanged line set (as in the sample), or measured cache hits. For a repeated, stable filtered lookup, keep a local dictionary for one operation, keyed by every input that affects the result (including company, filters, date, unit, currency, and quantity where applicable). Cache negative results as well as positive ones; distinguish a missing dictionary entry from an entry whose value is `false`. If underlying records can change during the run, update or invalidate the entry, or do not cache it. Bound entries or process in chunks when key cardinality is large. Check distinct complete keys, hits/misses, SQL work, AL time, and memory before adding a cache to a low-reuse workload. Use a temporary table for record-shaped values or multiple keys. See sample: [`cache-repeated-filtered-results-with-explicit-scope.good.al`](cache-repeated-filtered-results-with-explicit-scope.good.al). + +## Anti Pattern + +Running the same filtered `IsEmpty` in two passes over the same unchanged lines (even when every item number is distinct within a pass), or caching a price by item alone when customer, variant, date, and quantity affect it. Do **not** infer a cache opportunity from a single pass with no established key reuse, such as visiting each distinct Item once; the [good sample](cache-repeated-filtered-results-with-explicit-scope.good.al) also shows this valid direct lookup. Do not equate each `Get` with a SQL round trip or automatically wrap a cached primary-key read in another dictionary; see [primary-key cache exceptions](primary-key-get-in-loop-is-transaction-cached.md). See sample: [`cache-repeated-filtered-results-with-explicit-scope.bad.al`](cache-repeated-filtered-results-with-explicit-scope.bad.al). + +## References + +- [Data access and caching](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-data-access). +- [Dictionary type and `Get`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/dictionary/dictionary-data-type). diff --git a/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.bad.al b/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.bad.al index 48318a9..8153e70 100644 --- a/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.bad.al +++ b/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.bad.al @@ -1,15 +1,14 @@ codeunit 50223 "Perf Sample CalcSums Bad" { - procedure TotalRemaining(CustomerNo: Code[20]) Total: Decimal + procedure TotalSales(CustomerNo: Code[20]) Total: Decimal var CustLedgerEntry: Record "Cust. Ledger Entry"; begin + CustLedgerEntry.SetCurrentKey("Customer No."); CustLedgerEntry.SetRange("Customer No.", CustomerNo); - // One SQL query per row over a 10M-row ledger. if CustLedgerEntry.FindSet() then repeat - CustLedgerEntry.CalcFields("Remaining Amount"); - Total += CustLedgerEntry."Remaining Amount"; + Total += CustLedgerEntry."Sales (LCY)"; until CustLedgerEntry.Next() = 0; end; } diff --git a/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.good.al b/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.good.al index 2e1f367..2e41049 100644 --- a/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.good.al +++ b/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.good.al @@ -1,11 +1,12 @@ codeunit 50222 "Perf Sample CalcSums Good" { - procedure TotalRemaining(CustomerNo: Code[20]) Total: Decimal + procedure TotalSales(CustomerNo: Code[20]) Total: Decimal var CustLedgerEntry: Record "Cust. Ledger Entry"; begin + CustLedgerEntry.SetCurrentKey("Customer No."); CustLedgerEntry.SetRange("Customer No.", CustomerNo); - CustLedgerEntry.CalcSums("Remaining Amount"); - Total := CustLedgerEntry."Remaining Amount"; + CustLedgerEntry.CalcSums("Sales (LCY)"); + Total := CustLedgerEntry."Sales (LCY)"; end; } diff --git a/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.md b/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.md index f4d7ad9..fff5e25 100644 --- a/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.md +++ b/microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.md @@ -1,26 +1,31 @@ --- bc-version: [all] domain: performance -keywords: [calcfields, calcsums, loop, flowfield, n-plus-one, aggregation] +keywords: [calcfields, calcsums, loop, flowfield, source-field, sumindexfields, aggregation] technologies: [al] countries: [w1] application-area: [all] --- -# Use CalcSums to aggregate, not CalcFields inside a loop +# Use CalcSums for stored-field totals, not as a shortcut for FlowFields ## Description -`CalcFields` materializes FlowField values for one record. Each call against a persistent table is "a separate SQL query"; running it inside a `repeat ... until Next() = 0` over a large table issues one query per row on top of the iteration itself. `CalcSums` answers the same aggregation question — "give me the sum of this FlowField over the filtered set" — as a single SQL statement. Per the upstream guidance, `CalcFields` inside loops on large persistent tables is "a performance problem"; the aggregation form is `CalcSums()`. +`CalcFields` evaluates a FlowField for one record; `CalcSums` totals stored numeric fields in a filtered source table. They do not generally answer the same question. A loop that adds a normal source field for one total can often use one `CalcSums`, possibly backed by a compatible SIFT index. A loop that adds calculated FlowFields cannot be replaced with `CalcSums` on those FlowFields: each `CalcFormula` may depend on its parent record, FlowFilters, and the selected parent set. `CalcFields` requests can also use a recent calculation cache, so source-level call counts are not SQL statement counts. ## Best Practice -When the procedure totals a FlowField (or several) across a filtered set, set the filters, then call `CalcSums("Field 1", "Field 2", ...)`. The platform issues one query; the result is read off the record's FlowField slot. Single `CalcFields` outside loops is fine, and `CalcFields` on the current row in a page's `OnAfterGetRecord` or in `OnValidate` is the standard pattern — those are per-action, not per-row over a large set. +When only one total over stored source fields is needed, set the source-table filters and call `CalcSums` on those fields; select an appropriate current key with `SumIndexFields` when relying on SIFT, and measure the read/write trade-off. If the inputs are FlowFields, derive any proposed source aggregation from their `CalcFormula`, including the selected parent set and FlowFilters, and verify equivalent results before replacing the loop. When every row needs its own FlowField value, [use `SetAutoCalcFields`](use-setautocalcfields-for-per-row-flowfields.md) where appropriate rather than replacing row values with one total. See [SIFT trade-offs](choose-maintainsiftindex-by-read-write-ratio.md). See sample: [`calcsums-instead-of-calcfields-in-loop.good.al`](calcsums-instead-of-calcfields-in-loop.good.al). ## Anti Pattern -`if CustLedgerEntry.FindSet() then repeat CustLedgerEntry.CalcFields("Remaining Amount"); Total += CustLedgerEntry."Remaining Amount"; until CustLedgerEntry.Next() = 0;` — exactly the upstream-flagged shape. The iteration is the cheap part; the per-row `CalcFields` is what scales linearly with table size. +Looping over filtered `Cust. Ledger Entry` records and adding the stored `"Sales (LCY)"` field when the only output is its total. The opposite mistake is proposing `Customer.CalcSums(Balance)` as a generic replacement for adding selected customers' FlowField balances; that changes or fails to express the required calculation. See sample: [`calcsums-instead-of-calcfields-in-loop.bad.al`](calcsums-instead-of-calcfields-in-loop.bad.al). + +## References + +- [CalcFields and CalcSums operate on different field calculations](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-calcfields-calcsums-fielderror-fieldname-init-testfield-and-validate-methods). +- [Record.CalcSums](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-calcsums-method). diff --git a/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.bad.al b/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.bad.al new file mode 100644 index 0000000..7358289 --- /dev/null +++ b/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.bad.al @@ -0,0 +1,38 @@ +table 50360 "Perf Read Entry" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Customer No."; Code[20]) { } + field(3; "Posting Date"; Date) { } + field(4; "Item No."; Code[20]) { } + field(5; Quantity; Decimal) { } + field(6; Description; Text[100]) { } + } + + keys + { + key(PK; "Entry No.") { Clustered = true; } + key(ByDescription; Description, "Item No.", "Posting Date") { } + key(ByQuantity; Quantity, "Customer No.") { } + } +} + +codeunit 50361 "Perf Read Entry Bad" +{ + procedure SumNonblankItems(CustomerNo: Code[20]; FromDate: Date; ToDate: Date) Total: Decimal + var + Entry: Record "Perf Read Entry"; + begin + Entry.SetRange("Customer No.", CustomerNo); + Entry.SetRange("Posting Date", FromDate, ToDate); + Entry.SetLoadFields("Item No.", Quantity); + if Entry.FindSet() then + repeat + if Entry."Item No." <> '' then + Total += Entry.Quantity; + until Entry.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.good.al b/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.good.al new file mode 100644 index 0000000..17ea82d --- /dev/null +++ b/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.good.al @@ -0,0 +1,41 @@ +table 50360 "Perf Read Entry" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Customer No."; Code[20]) { } + field(3; "Posting Date"; Date) { } + field(4; "Item No."; Code[20]) { } + field(5; Quantity; Decimal) { } + field(6; Description; Text[100]) { } + } + + keys + { + key(PK; "Entry No.") { Clustered = true; } + key(ByCustomerDate; "Customer No.", "Posting Date") + { + // Supports the filters and explicit payload; implicit system fields may still require lookups. + IncludedFields = "Item No.", Quantity; + } + } +} + +codeunit 50361 "Perf Read Entry Good" +{ + procedure SumNonblankItems(CustomerNo: Code[20]; FromDate: Date; ToDate: Date) Total: Decimal + var + Entry: Record "Perf Read Entry"; + begin + Entry.SetRange("Customer No.", CustomerNo); + Entry.SetRange("Posting Date", FromDate, ToDate); + Entry.SetLoadFields("Item No.", Quantity); + if Entry.FindSet() then + repeat + if Entry."Item No." <> '' then + Total += Entry.Quantity; + until Entry.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.md b/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.md new file mode 100644 index 0000000..0650086 --- /dev/null +++ b/microsoft/knowledge/performance/design-covering-keys-from-read-pattern.md @@ -0,0 +1,30 @@ +--- +bc-version: [19..] +domain: performance +keywords: [includedfields, covering-index, secondary-key, filter, projection, selectivity, key, setrange] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Design a read-pattern key and verify whether it covers the query + +## Description + +A secondary key should serve a particular read, not a list of fields that happen to look important. The order of key fields affects which filters and sort orders it can support; `IncludedFields` supplies non-key payload columns without making them ordered key columns. `SetCurrentKey` sets an order, not an index hint (see [sort guidance](setcurrentkey-sets-sort-order-not-index-hint.md)). A key that supports the predicates is not necessarily a covering index, and coverage alone does not make a query selective. + +## Best Practice + +For a costly, frequent read, establish its equality and range filters, joins, required ordering, actual SQL projection, cardinality, and existing physical indexes. Test a key whose leading fields support the useful predicates; for example, customer equality followed by a posting-date range. On a nonclustered secondary key, consider `IncludedFields` for small payload fields read but not filtered or ordered. The sample's key supports customer/date filters and includes the explicit payload, but is **not demonstrated to cover the read**: Business Central also projects `SystemId` and system audit fields on partial records. Check the generated SQL, including automatically selected, clustered-key, and extension fields, against the physical index before claiming coverage. Adding more included fields has storage and write-maintenance costs; measure reads, sorts, lookups, latency, and writes before expanding it. The [Database Missing Indexes page](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/database-missing-indexes) supplies candidates, not a mandate to add every suggested key. + +`IncludedFields` requires runtime 8.0 (BC 19) or later and cannot be set on a primary or clustered secondary key. An included field does not participate in `SetCurrentKey` matching or maintain a SIFT sum. If the item is also a selective predicate or required ordering column, evaluate it as a key field instead. Respect table-extension key field-ownership restrictions. See sample: [`design-covering-keys-from-read-pattern.good.al`](design-covering-keys-from-read-pattern.good.al). + +## Anti Pattern + +Adding a key on output-only fields or requesting an unrelated `SetCurrentKey` ordering to "force" the optimizer to use that key, without establishing the read's filters or validating its plan. Likewise, calling an index covering just because it contains the fields explicitly listed in `SetLoadFields` ignores automatically projected columns. Do not report every uncovered read as a defect: a small table, a low-frequency query, or a write-heavy table may be better without another maintained index. See sample: [`design-covering-keys-from-read-pattern.bad.al`](design-covering-keys-from-read-pattern.bad.al). + +## References + +- [Table keys, included columns, and extension restrictions](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-table-keys). +- [IncludedFields property](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-includedfields-property). +- [Table keys and performance](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-table-keys-and-performance). diff --git a/microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.md b/microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.md index c88cdfe..cdbd8ae 100644 --- a/microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.md +++ b/microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.md @@ -13,11 +13,11 @@ application-area: [all] ## Description -`IsEmpty` is the right API when the caller only needs existence — see `microsoft/knowledge/performance/use-isempty-for-existence-check.md`. It is not a cheap guard in front of a loop that will `FindSet` anyway. Both calls hit the database; `FindSet` already returns false when the filter matches nothing. Agents and reviewers often insert `if not Rec.IsEmpty() then` "for performance" and pay a second query for a result the iterator already provides. +`IsEmpty` is the right API when the caller only needs existence — see `microsoft/knowledge/performance/use-isempty-for-existence-check.md`. It is not a cheap guard in front of a loop that will `FindSet` anyway. `FindSet` already returns false when the filter matches nothing; an extra `IsEmpty` is unnecessary AL work and can issue a second database request, depending on caching. Agents and reviewers often insert `if not Rec.IsEmpty() then` "for performance" and duplicate a result the iterator already provides. ## Best Practice -When the body iterates, open with `if Rec.FindSet() then repeat ... until Next() = 0`. Do not flag a bare `FindSet` loop as missing an `IsEmpty` precondition. Reserve `IsEmpty` for branches that never materialize the row set. +When the body iterates, open with `if Rec.FindSet() then repeat ... until Next() = 0`. Do not flag a bare `FindSet` loop as missing an `IsEmpty` precondition. Reserve `IsEmpty` for branches that never materialize the row set. An early check before a *bulk write* or lock is a different, workload-dependent decision: it can help when the filtered set is usually empty, but adds work when rows exist and does not lock the set against change. See sample: [`isempty-before-findset-is-extra-round-trip.good.al`](isempty-before-findset-is-extra-round-trip.good.al). diff --git a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.good.al b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.good.al index 9a3ad4c..1c0fa22 100644 --- a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.good.al +++ b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.good.al @@ -20,6 +20,7 @@ codeunit 50242 "Perf Sample ModifyAll Good" StagingEntry: Record "Perf Import Staging Entry"; begin StagingEntry.SetRange("Batch ID", BatchId); + StagingEntry.SetRange(Processed, false); // Processed has no OnValidate logic, and the equivalent loop uses Modify(false). StagingEntry.ModifyAll(Processed, true, false); end; diff --git a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md index ad7a6cf..34ea81c 100644 --- a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md +++ b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md @@ -15,7 +15,7 @@ application-area: [all] ## Best Practice -Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics. +Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. For a data-only update, exclude rows already holding the target value when that filter preserves the business outcome. Do not skip unchanged rows if the original call's per-row effects are required. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics. See sample: [`prefer-modifyall-over-per-row-modify.good.al`](prefer-modifyall-over-per-row-modify.good.al). diff --git a/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.bad.al b/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.bad.al new file mode 100644 index 0000000..d6c02c3 --- /dev/null +++ b/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.bad.al @@ -0,0 +1,61 @@ +// Each invocation exclusively owns a new run; these tables have no write logic. +table 50364 "Perf Input" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Row No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + } + + keys + { + key(PK; "Row No.") { Clustered = true; } + } +} + +table 50365 "Perf Output" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Run ID"; Guid) { } + field(2; "Line No."; Integer) { } + field(3; "Item No."; Code[20]) { } + field(4; Quantity; Decimal) { } + } + + keys + { + key(PK; "Run ID", "Line No.") { Clustered = true; } + } +} + +codeunit 50366 "Perf Buffered Insert Bad" +{ + procedure CopyRun(var TempInput: Record "Perf Input" temporary) RunId: Guid + var + Output: Record "Perf Output"; + NextLineNo: Integer; + begin + RunId := CreateGuid(); + Output.SetRange("Run ID", RunId); + if TempInput.FindSet() then + repeat + if Output.FindLast() then + NextLineNo := Output."Line No." + 1 + else + NextLineNo := 1; + Output.Init(); + Output."Run ID" := RunId; + Output."Line No." := NextLineNo; + Output."Item No." := TempInput."Item No."; + Output.Insert(false); + Output.Quantity := TempInput.Quantity; + Output.Modify(false); + until TempInput.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.good.al b/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.good.al new file mode 100644 index 0000000..96d0faa --- /dev/null +++ b/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.good.al @@ -0,0 +1,56 @@ +// Each invocation exclusively owns a new run; these tables have no write logic. +table 50364 "Perf Input" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Row No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + } + + keys + { + key(PK; "Row No.") { Clustered = true; } + } +} + +table 50365 "Perf Output" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Run ID"; Guid) { } + field(2; "Line No."; Integer) { } + field(3; "Item No."; Code[20]) { } + field(4; Quantity; Decimal) { } + } + + keys + { + key(PK; "Run ID", "Line No.") { Clustered = true; } + } +} + +codeunit 50366 "Perf Buffered Insert Good" +{ + procedure CopyRun(var TempInput: Record "Perf Input" temporary) RunId: Guid + var + Output: Record "Perf Output"; + NextLineNo: Integer; + begin + RunId := CreateGuid(); + if TempInput.FindSet() then + repeat + NextLineNo += 1; + Output.Init(); + Output."Run ID" := RunId; + Output."Line No." := NextLineNo; + Output."Item No." := TempInput."Item No."; + Output.Quantity := TempInput.Quantity; + Output.Insert(false); + until TempInput.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.md b/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.md new file mode 100644 index 0000000..7459ee4 --- /dev/null +++ b/microsoft/knowledge/performance/preserve-buffered-inserts-by-separating-target-reads.md @@ -0,0 +1,27 @@ +--- +bc-version: [all] +domain: performance +keywords: [buffered-inserts, bulk-inserts, findlast, insert, target-table, commit, staging] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep eligible inserts together instead of re-reading their target + +## Description + +Business Central can automatically buffer eligible `Insert` calls. `Find`/`Calc` on the **target** table, `Modify`/`Delete` on it, or `Commit` flushes pending inserts; consuming the `Insert` return value, or BLOB/AutoIncrement fields on the target, prevents buffering. A source-table read is not itself a target-table flush. There is no general `InsertAll` replacement for an AL loop. + +## Best Practice + +When writing completed rows to an application-owned data-only table, allocate a collision-free run or range once, prepare every field before each `Insert`, and keep the insert sequence free of intervening target-table reads and writes. Let the owning business transaction determine the commit point. Trace called procedures and events as well as the visible loop; inspect actual SQL batches and writes, then test failures, retries, and any concurrent writers. The sample assumes an exclusive new run ID and no required trigger, validation, number-series, or subscriber effects. See sample: [`preserve-buffered-inserts-by-separating-target-reads.good.al`](preserve-buffered-inserts-by-separating-target-reads.good.al). + +## Anti Pattern + +Calling `FindLast` on the target for every row to allocate its next line number, inserting an incomplete row and immediately modifying it, or committing each iteration of an otherwise eligible insert sequence. Moving a shared `FindLast` out of the loop without concurrency-safe allocation is **not** a valid fix. Do not recommend skipping required triggers or persistence steps in sales documents, journals, or posting ledgers just to obtain buffering; repeated `Modify` calls do not automatically batch like eligible inserts. See sample: [`preserve-buffered-inserts-by-separating-target-reads.bad.al`](preserve-buffered-inserts-by-separating-target-reads.bad.al). + +## References + +- [Bulk inserts and flush/non-buffering conditions](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-bulk-inserts). +- [Transaction checkpoints and restart safety](avoid-commit-inside-loops.md). diff --git a/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md b/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md index ed41f1b..3594a05 100644 --- a/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md +++ b/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md @@ -13,12 +13,12 @@ application-area: [all] The Business Central server caches primary-key reads within a transaction. Repeated `Record.Get()` calls for the same key are served from that cache rather than re-queried, so a guarded `if not Rec.Get(...) then exit;` inside a per-row helper is not a genuine N+1 pattern. When each row legitimately carries a distinct key — for example one `Bin Content` row per bin, so `Bin.Get` and `BinType.Get` see a different bin each iteration — the `Get` must run per row regardless, and there is nothing to hoist. -Reviewers sometimes see two `Get` calls inside a routine that runs once per row and recommend wrapping them in a `Dictionary` cache. That is over-engineering: it duplicates the server's built-in record cache, adds state that must be invalidated, and breaks the surrounding extension's established pattern of direct guarded `Get` calls. +Reviewers sometimes see two `Get` calls inside a routine that runs once per row and recommend wrapping them in a `Dictionary` cache. Without evidence of a material additional cost, that duplicates the server's built-in record cache and adds state that must be invalidated. Filtered, non-keyed reads are a different case (see [repeated filtered results](cache-repeated-filtered-results-with-explicit-scope.md)). ## Best Practice -Treat a primary-key `Get()` — especially a guarded `if not Rec.Get(...) then exit;` — as a cheap, transaction-cached read. Do not recommend a manual `Dictionary` cache around per-row primary-key `Get` calls. Reserve N+1 concerns for genuinely repeated non-keyed queries (`FindSet`/`FindFirst` with filters, `Count`) that re-hit the database each iteration. +Treat a primary-key `Get()` — especially a guarded `if not Rec.Get(...) then exit;` — as a transaction-cached read, not as proof of N+1 SQL. Do not recommend a manual cache solely from source-level call counts. If profiling shows repeated AL work or cache misses are material and the complete result can be reused safely, assess an explicitly scoped cache on its own merits. Investigate genuinely repeated non-keyed queries (`FindSet`/`FindFirst` with filters, `Count`) separately. ## Anti Pattern -Reporting repeated primary-key `Get` calls (such as `Bin.Get` and `BinType.Get`) inside a per-row helper as a performance defect, or recommending they be cached in a `Dictionary`. The reads are already cached by the server within the transaction, and per-row keys often differ so the calls cannot be hoisted. +Reporting repeated primary-key `Get` calls (such as `Bin.Get` and `BinType.Get`) inside a per-row helper as one SQL round trip per call, or recommending a `Dictionary` without measuring reuse and cost. Per-row keys often differ, and the server can satisfy repeated keys from its transaction cache. diff --git a/microsoft/knowledge/performance/query-results-bypass-primary-key-cache.md b/microsoft/knowledge/performance/query-results-bypass-primary-key-cache.md index 5bcfbd8..52f590c 100644 --- a/microsoft/knowledge/performance/query-results-bypass-primary-key-cache.md +++ b/microsoft/knowledge/performance/query-results-bypass-primary-key-cache.md @@ -13,7 +13,7 @@ application-area: [all] ## Description -The Business Central server caches primary-key `Get` calls within a transaction. Query objects do not use that cache: every `Open`/`Read` goes to SQL. `avoid-get-inside-loop-on-large-table.md` is right when an unbounded inner `Get`/`FindFirst` joins two large sets. It is wrong as a blanket rewrite of repeated `Get` on the same keys. Replacing a cached `Get` with a Query that re-executes per call can be slower. This file exists so reviewers stop treating every `Get` inside a loop as a Query candidate. +The Business Central server caches primary-key `Get` calls within a transaction. Query objects do not use that primary-key cache: reopening a Query per lookup executes the query again; `Read` consumes rows from an open query, not a new query for each row. `avoid-get-inside-loop-on-large-table.md` is right when an unbounded inner `Get`/`FindFirst` joins two large sets. It is wrong as a blanket rewrite of repeated `Get` on the same keys. Replacing a cached `Get` with a Query reopened per call can be slower. ## Best Practice diff --git a/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.bad.al b/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.bad.al new file mode 100644 index 0000000..7ad16a6 --- /dev/null +++ b/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.bad.al @@ -0,0 +1,43 @@ +// The only supported read filters customer/date and displays item, quantity, and amount. +// No consumer needs ordering on the displayed fields or a SIFT aggregate. +table 50362 "Perf Document Entry" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Customer No."; Code[20]) { } + field(3; "Posting Date"; Date) { } + field(4; "Item No."; Code[20]) { } + field(5; Quantity; Decimal) { } + field(6; Amount; Decimal) { } + } + + keys + { + key(PK; "Entry No.") { Clustered = true; } + key(CustomerDate; "Customer No.", "Posting Date") { } + key(CustomerDateItem; "Customer No.", "Posting Date", "Item No.") { } + key(CustomerDateQuantity; "Customer No.", "Posting Date", Quantity) { } + key(CustomerDateAmount; "Customer No.", "Posting Date", Amount) { } + } +} + +codeunit 50375 "Perf Document Reader Bad" +{ + procedure CustomerDateTotals(CustomerNo: Code[20]; FromDate: Date; ToDate: Date; var ItemNos: List of [Code[20]]; var TotalAmount: Decimal; var TotalQuantity: Decimal) + var + Entry: Record "Perf Document Entry"; + begin + Entry.SetRange("Customer No.", CustomerNo); + Entry.SetRange("Posting Date", FromDate, ToDate); + Entry.SetLoadFields("Item No.", Quantity, Amount); + if Entry.FindSet() then + repeat + ItemNos.Add(Entry."Item No."); + TotalQuantity += Entry.Quantity; + TotalAmount += Entry.Amount; + until Entry.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.good.al b/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.good.al new file mode 100644 index 0000000..8608396 --- /dev/null +++ b/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.good.al @@ -0,0 +1,43 @@ +// The only supported read filters customer/date and displays item, quantity, and amount. +// No consumer needs ordering on the displayed fields or a SIFT aggregate. +table 50362 "Perf Document Entry" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Customer No."; Code[20]) { } + field(3; "Posting Date"; Date) { } + field(4; "Item No."; Code[20]) { } + field(5; Quantity; Decimal) { } + field(6; Amount; Decimal) { } + } + + keys + { + key(PK; "Entry No.") { Clustered = true; } + key(CustomerDatePayload; "Customer No.", "Posting Date") + { + IncludedFields = "Item No.", Quantity, Amount; + } + } +} + +codeunit 50375 "Perf Document Reader Good" +{ + procedure CustomerDateTotals(CustomerNo: Code[20]; FromDate: Date; ToDate: Date; var ItemNos: List of [Code[20]]; var TotalAmount: Decimal; var TotalQuantity: Decimal) + var + Entry: Record "Perf Document Entry"; + begin + Entry.SetRange("Customer No.", CustomerNo); + Entry.SetRange("Posting Date", FromDate, ToDate); + Entry.SetLoadFields("Item No.", Quantity, Amount); + if Entry.FindSet() then + repeat + ItemNos.Add(Entry."Item No."); + TotalQuantity += Entry.Quantity; + TotalAmount += Entry.Amount; + until Entry.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.md b/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.md new file mode 100644 index 0000000..61ed36e --- /dev/null +++ b/microsoft/knowledge/performance/review-overlapping-keys-before-adding-an-index.md @@ -0,0 +1,29 @@ +--- +bc-version: [all] +domain: performance +keywords: [overlapping-keys, redundant-index, includedfields, sift, write-amplification, index-portfolio, key, setrange] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Review overlapping keys as a portfolio + +## Description + +Adding a secondary key to a frequently written table maintains another SQL index for every affected write. Several keys with the same leading fields might be serving distinct filters, sort orders, unique constraints, or SIFT aggregates; they might instead be redundant payload variants. A shared prefix, absent `SetCurrentKey` calls, or low usage in a short window is not proof that a key is unused. + +## Best Practice + +Before adding or removing a key, inventory keys from the table and installed extensions and identify each key's consumers and purpose: seek, ordering, uniqueness, or aggregation. Check `SQLIndex`, `MaintainSQLIndex`, `SumIndexFields`, and `MaintainSIFTIndex`, not only the AL key name. For *confirmed* payload-only variants on BC 19 or later, a single nonclustered key with `IncludedFields` can be a consolidation candidate (see [read-pattern key design](design-covering-keys-from-read-pattern.md)); an included field cannot replace a key column used for ordering or a maintained SIFT sum. Including the explicit payload does not prove that the resulting index covers every automatically selected field. Measure representative read and write workloads, including periodic reports, integrations, and other companies, before and after a supported extension/schema change. Retain a rollback path for a critical reader that regresses. See sample: [`review-overlapping-keys-before-adding-an-index.good.al`](review-overlapping-keys-before-adding-an-index.good.al). + +## Anti Pattern + +Keeping another customer/date key for each displayed field without checking whether the trailing fields support distinct operations. Conversely, merging `(Customer, Date)` with `(Customer, Item, Date)` merely because they share a prefix can regress item-selective queries; removing a unique key or a SIFT aggregate changes more than write cost. Do not report a key as unused solely because AL never calls `SetCurrentKey` on it. See sample: [`review-overlapping-keys-before-adding-an-index.bad.al`](review-overlapping-keys-before-adding-an-index.bad.al). + +## References + +- [Table keys: benefits, costs, and constraints](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-table-keys). +- [IncludedFields property](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-includedfields-property). +- [Manage index usage (BC 2026 release wave 1 and later)](https://learn.microsoft.com/en-us/dynamics365/business-central/manage-indexes). +- [SIFT read/write trade-off](choose-maintainsiftindex-by-read-write-ratio.md). diff --git a/microsoft/knowledge/performance/setcurrentkey-sets-sort-order-not-index-hint.md b/microsoft/knowledge/performance/setcurrentkey-sets-sort-order-not-index-hint.md index ad0bb40..ea99299 100644 --- a/microsoft/knowledge/performance/setcurrentkey-sets-sort-order-not-index-hint.md +++ b/microsoft/knowledge/performance/setcurrentkey-sets-sort-order-not-index-hint.md @@ -7,11 +7,11 @@ countries: [w1] application-area: [all] --- -# SetCurrentKey only sets sort order — it is not an index hint +# SetCurrentKey is not a SQL index hint for record reads ## Description -A common misconception is that `SetCurrentKey` tells SQL Server which index to use for a query. It does not. In Business Central, `SetCurrentKey` only changes the `ORDER BY` clause of the generated SQL statement. It does not add an index hint, and the SQL Server query optimizer is free to ignore the named key entirely. +A common misconception is that `SetCurrentKey` tells SQL Server which index to use for a filtered `FindSet`/`FindFirst` query. It does not. For record iteration, `SetCurrentKey` changes the `ORDER BY` clause of the generated SQL statement; it does not add an index hint, and the SQL Server query optimizer is free to ignore the named key entirely. A distinct use is selecting an appropriate key with `SumIndexFields` before `CalcSums` so the platform can use a compatible SIFT aggregate (see [SIFT and SQL Server](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-sift-and-sql-server)). The optimizer picks the index from the `WHERE` clause (your `SetRange`/`SetFilter`) together with table statistics and estimated cost. In practice it almost never chooses an index just because that key appears in `ORDER BY`. So calling `SetCurrentKey` to "steer" the plan toward an index is a no-op for index selection — and can make things worse: an `ORDER BY` that the query does not otherwise need can push the optimizer toward a less selective index or add a Sort operator to the plan. @@ -19,13 +19,15 @@ Selectivity comes from having the right index available (a key on the table whos ## Best Practice -Decide `SetCurrentKey` on one question only: **do I need the result set in a specific order?** +For a record-iteration read, decide `SetCurrentKey` on one question: **do I need the result set in a specific order?** - If yes — you iterate rows in a defined sequence, or rely on `FindFirst`/`FindLast`/`Next` returning a particular row — call `SetCurrentKey` for that sort. The order is a functional requirement, and the `ORDER BY` is justified. - If no — omit `SetCurrentKey`. Let the optimizer choose the cheapest plan for your filters; it may pick a better index and skip a sort. To make a filtered read fast, ensure a key (index) exists on the table whose leading fields cover the filter, and filter on those fields with `SetRange`/`SetFilter`. That is what lets the optimizer seek. Defining the key creates the index; `SetCurrentKey` is not required to make the optimizer use it. +For a stored-field `CalcSums` instead of iteration, consider a matching current key with the summed field in `SumIndexFields`, as documented for SIFT; do not classify that call as an unnecessary `ORDER BY` on a row iterator (see [stored-field totals](calcsums-instead-of-calcfields-in-loop.md)). + See sample: [`setcurrentkey-sets-sort-order-not-index-hint.good.al`](setcurrentkey-sets-sort-order-not-index-hint.good.al). ## Anti Pattern diff --git a/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.bad.al b/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.bad.al new file mode 100644 index 0000000..f3de8f5 --- /dev/null +++ b/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.bad.al @@ -0,0 +1,39 @@ +// Caller supplies an unfiltered temporary buffer that does not change during this call. +table 50373 "Perf Cell" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Cell No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + } + + keys + { + key(PK; "Cell No.") { Clustered = true; } + } +} + +codeunit 50374 "Perf Temp Totals Bad" +{ + procedure SumDisplayedItemTotals(var TempCells: Record "Perf Cell" temporary) DisplayedTotal: Decimal + var + TempScan: Record "Perf Cell" temporary; + ItemTotal: Decimal; + begin + TempScan.Copy(TempCells, true); + if TempCells.FindSet() then + repeat + TempScan.Reset(); + TempScan.SetRange("Item No.", TempCells."Item No."); + ItemTotal := 0; + if TempScan.FindSet() then + repeat + ItemTotal += TempScan.Quantity; + until TempScan.Next() = 0; + DisplayedTotal += ItemTotal; + until TempCells.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.good.al b/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.good.al new file mode 100644 index 0000000..c02da0c --- /dev/null +++ b/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.good.al @@ -0,0 +1,39 @@ +// Caller supplies an unfiltered temporary buffer that does not change during this call. +table 50373 "Perf Cell" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Cell No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + } + + keys + { + key(PK; "Cell No.") { Clustered = true; } + } +} + +codeunit 50374 "Perf Temp Totals Good" +{ + procedure SumDisplayedItemTotals(var TempCells: Record "Perf Cell" temporary) DisplayedTotal: Decimal + var + TotalsByItem: Dictionary of [Code[20], Decimal]; + ItemTotal: Decimal; + begin + if TempCells.FindSet() then + repeat + if TotalsByItem.Get(TempCells."Item No.", ItemTotal) then + TotalsByItem.Set(TempCells."Item No.", ItemTotal + TempCells.Quantity) + else + TotalsByItem.Add(TempCells."Item No.", TempCells.Quantity); + until TempCells.Next() = 0; + + if TempCells.FindSet() then + repeat + DisplayedTotal += TotalsByItem.Get(TempCells."Item No."); + until TempCells.Next() = 0; + end; +} diff --git a/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.md b/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.md index 3fd8072..a392e0b 100644 --- a/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.md +++ b/microsoft/knowledge/performance/temporary-tables-have-no-database-cost.md @@ -15,8 +15,12 @@ A temporary table stores its rows in Business Central Server memory instead of a ## Best Practice -Do not apply SQL-specific findings such as missing `SetLoadFields`, lock contention, or N+1 database round-trips to a temporary record. Still assess memory volume and repeated scans or lookups. For a pure key-to-value collection, consider an AL `Dictionary`; keep a temporary table when record fields, keys, filtering, or ordered iteration are required. +Do not apply SQL-specific findings such as missing `SetLoadFields`, lock contention, or N+1 database round-trips to a temporary record. Still assess memory volume and repeated scans or lookups. In a matrix, if each cell re-sums the same group, calculate the totals once at the complete group key and reuse them; invalidate or adjust totals if cells change. For a pure key-to-value collection, consider an AL `Dictionary`; keep a temporary table when record fields, keys, filtering, or ordered iteration are required. Measure AL time and peak memory, not just SQL time. + +See sample: [`temporary-tables-have-no-database-cost.good.al`](temporary-tables-have-no-database-cost.good.al). ## Anti Pattern Claiming that every temporary-table access pattern is free because no SQL is involved. A nested scan over a large in-memory buffer can still dominate service-tier CPU, while adding `SetLoadFields` to that buffer addresses a database cost that does not exist. + +See sample: [`temporary-tables-have-no-database-cost.bad.al`](temporary-tables-have-no-database-cost.bad.al). diff --git a/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.md b/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.md index b5f9d6a..e23cc98 100644 --- a/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.md +++ b/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.md @@ -15,12 +15,12 @@ application-area: [all] ## Best Practice -Call `SetAutoCalcFields` before `FindSet` when every returned row needs the same FlowField for a comparison, branch, or per-record action. Use `CalcSums` instead when the required result is one aggregate over the filtered set (see `calcsums-instead-of-calcfields-in-loop.md`). +Call `SetAutoCalcFields` before `FindSet` when every returned row needs the same FlowField for a comparison, branch, or per-record action. For one total of a **stored source field**, consider `CalcSums` instead (see [stored-field totals versus FlowFields](calcsums-instead-of-calcfields-in-loop.md)). If the required result is a total of selected FlowField values, derive the underlying source filters from `CalcFormula`; do not sum the FlowField directly with `CalcSums`. See sample: [`use-setautocalcfields-for-per-row-flowfields.good.al`](use-setautocalcfields-for-per-row-flowfields.good.al). ## Anti Pattern -Calling `CalcFields` inside the loop when every iteration reads the same FlowField. Each `CalcFields` request requires a separate SQL statement unless a compatible recent result is cached. Do not replace row-specific decisions with `CalcSums`; an aggregate cannot preserve which rows met the condition. +Calling `CalcFields` inside the loop when every iteration reads the same FlowField. A compatible recent result can be cached, so do not equate each call with a SQL statement. Do not replace row-specific decisions with `CalcSums`; an aggregate cannot preserve which rows met the condition. See sample: [`use-setautocalcfields-for-per-row-flowfields.bad.al`](use-setautocalcfields-for-per-row-flowfields.bad.al). diff --git a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.bad.al b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.bad.al index c9b4ce2..c7054f3 100644 --- a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.bad.al +++ b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.bad.al @@ -1,14 +1,18 @@ codeunit 50219 "Perf Sample LoadFields Bad" { - procedure ListUSCustomerNames() + procedure CollectUSCustomerNamesAndCities(var DisplayNames: List of [Text]) var Customer: Record Customer; begin - // Loads every Customer column on every row, when only Name is read. Customer.SetRange("Country/Region Code", 'US'); if Customer.FindSet() then repeat - Message(Customer.Name); + DisplayNames.Add(CustomerDisplayText(Customer)); until Customer.Next() = 0; end; + + local procedure CustomerDisplayText(Customer: Record Customer): Text + begin + exit(Customer.Name + ' ' + Customer.City); + end; } diff --git a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al index a5bee5f..c663bc4 100644 --- a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al +++ b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al @@ -1,17 +1,22 @@ codeunit 50218 "Perf Sample LoadFields Good" { - procedure ListUSCustomerNames() + procedure CollectUSCustomerNamesAndCities(var DisplayNames: List of [Text]) var Customer: Record Customer; begin Customer.SetRange("Country/Region Code", 'US'); - Customer.SetLoadFields(Name); + Customer.SetLoadFields(Name, City); if Customer.FindSet() then repeat - Message(Customer.Name); + DisplayNames.Add(CustomerDisplayText(Customer)); until Customer.Next() = 0; end; + local procedure CustomerDisplayText(Customer: Record Customer): Text + begin + exit(Customer.Name + ' ' + Customer.City); + end; + procedure LookupSkuPolicy(LocationCode: Code[10]) Policy: Enum "SKU Creation Method" var Location: Record Location; diff --git a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md index de92bb1..5bd9bb7 100644 --- a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md +++ b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md @@ -17,7 +17,7 @@ Its position relative to `SetRange`/`SetFilter` does not change the projection: ## Best Practice -Before a `Get`, `FindSet`, or `FindFirst` that the procedure follows by reading only a handful of the table's fields, call `SetLoadFields` listing exactly those fields. For example, `SetLoadFields(...); if Record.Get(...) then ...` selects fields before the read. Place the call immediately before the read, after any `SetRange`/`SetFilter`, so a reader can see at a glance which read the selection governs and any projection-changing operation is easy to spot. Skip `SetLoadFields` when the table has few fields (under ten), when the code reads most of them (above 60 %), when the loop runs ten or fewer iterations, or when the table is exempt for other reasons ([singleton setup tables](singleton-setup-tables-need-no-access-optimization.md), [temporary tables](temporary-tables-have-no-database-cost.md)). The numeric cutoffs are BCQuality review heuristics, not Microsoft platform thresholds. For report dataitems, use `AddLoadFields` in `OnPreDataItem` instead (see [report partial loads](addloadfields-in-report-onpredataitem.md)). +Before a `Get`, `FindSet`, or `FindFirst` that the complete read path follows by reading only a handful of the table's fields, call `SetLoadFields` listing the normal fields used by the caller **and its helpers**. For example, `SetLoadFields(...); if Record.Get(...) then ...` selects fields before the read. Place the call immediately before the read, after any `SetRange`/`SetFilter`, so a reader can see at a glance which read the selection governs and any projection-changing operation is easy to spot. Check for later `Reset` and for unloaded fields read by a [by-value helper](pass-var-record-to-preserve-partial-load-enumerator.md); a JIT load can be served from cache, so it is not automatically a SQL round trip. Skip `SetLoadFields` when the table has few fields (under ten), when the code reads most of them (above 60 %), when the loop runs ten or fewer iterations, or when the table is exempt for other reasons ([singleton setup tables](singleton-setup-tables-need-no-access-optimization.md), [temporary tables](temporary-tables-have-no-database-cost.md)). The numeric cutoffs are BCQuality review heuristics, not Microsoft platform thresholds. For report dataitems, use `AddLoadFields` in `OnPreDataItem` instead (see [report partial loads](addloadfields-in-report-onpredataitem.md)). See sample: [`use-setloadfields-for-partial-records.good.al`](use-setloadfields-for-partial-records.good.al). diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al new file mode 100644 index 0000000..1a93861 --- /dev/null +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al @@ -0,0 +1,12 @@ +permissionset 50100 "Sample - Integration" +{ + Access = Public; + Assignable = false; + Caption = 'Sample Integration'; + Permissions = + tabledata "Sample Order" = RIMD; + // BUG: "Sample Order API" (PageType = API) and "Sample Order Query" + // (a published API query) have no "= X" entry anywhere in this app. + // Both endpoints are unreachable even though the table looks fully + // granted - nobody decided who may call them. +} diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al new file mode 100644 index 0000000..de8d2ab --- /dev/null +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al @@ -0,0 +1,10 @@ +permissionset 50100 "Sample - Integration" +{ + Access = Public; + Assignable = false; + Caption = 'Sample Integration'; + Permissions = + tabledata "Sample Order" = RIMD, + page "Sample Order API" = X, // exposed API page: execute granted + query "Sample Order Query" = X; // exposed API query: execute granted +} diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md new file mode 100644 index 0000000..fd933d7 --- /dev/null +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: security +keywords: [permission-set, api-page, web-service, exposure, access-control] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Every exposed object must belong to a permission set + +## Description + +An object that is reachable from outside the app's own UI is only usable if it is also granted execute access through a permission set. Three distinct mechanisms make an object reachable this way, each with its own permission target: + +- A page or query published through the **Web Services** configuration page, or a custom REST endpoint declared with `PageType = API` / `QueryType = API` — both need a `page "..." = X` / `query "..." = X` entry for that object. +- A codeunit published through **Web Services** exposes *every* public procedure on it as a SOAP operation automatically — there is no per-method attribute to add. SOAP web service support is deprecated and scheduled for removal; prefer publishing an API page/query for a new integration rather than a new codeunit web service. The permission target for an existing published codeunit is the codeunit itself: `codeunit "..." = X`. +- `[ServiceEnabled]` is a method-level attribute used on a *page* procedure to expose it as an OData v4 bound action (for example a `Post` action on an invoice page) — it does not apply to pages, queries, or codeunits as an object-level property, and it does not create its own permission target. The action is still a call into that page object, so the page's own `page "..." = X` entry is what governs it. + +When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing. + +## Best Practice + +Give every exposed object an explicit execute entry in a permission set shipped by the app: `page "..." = X` / `query "..." = X` for a published or API page/query (including one that exposes a `[ServiceEnabled]` bound action), and `codeunit "..." = X` for a codeunit published as a web service. Route sensitive endpoints into a dedicated, non-default admin permission set so reaching them requires a deliberate grant rather than being included by default. If an object should never be reachable from outside the app, remove the exposure itself (drop `PageType = API` / the Web Services registration) rather than leaving an orphaned endpoint with no permission-set membership. + +See sample: [`exposed-objects-must-be-in-a-permission-set.good.al`](exposed-objects-must-be-in-a-permission-set.good.al). + +## Anti Pattern + +Granting access to the underlying table data while forgetting to grant execute access to the exposed page or query itself. The table looks fully covered by a permission set, but the API/service layer in front of it has no `= X` entry anywhere, so the endpoint silently fails for every caller even though the data permissions look complete. + +See sample: [`exposed-objects-must-be-in-a-permission-set.bad.al`](exposed-objects-must-be-in-a-permission-set.bad.al). diff --git a/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al new file mode 100644 index 0000000..869e398 --- /dev/null +++ b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al @@ -0,0 +1,18 @@ +codeunit 50101 "Credit Memo Routing" +{ + procedure PostSalesLine(var SalesHeader: Record "Sales Header"; var SalesLine: Record "Sales Line"; CustomerNo: Code[20]; Amount: Decimal) + begin + // Set the customer number + SalesHeader.Validate("Sell-to Customer No.", CustomerNo); + // Insert the line + SalesLine.Insert(true); + // Check if the amount is positive + if Amount > 0 then + // Post the entry + PostEntry(Amount); + end; + + local procedure PostEntry(Amount: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al new file mode 100644 index 0000000..6bd39b1 --- /dev/null +++ b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al @@ -0,0 +1,20 @@ +codeunit 50101 "Credit Memo Routing" +{ + procedure PostSalesLine(var SalesHeader: Record "Sales Header"; var SalesLine: Record "Sales Line"; CustomerNo: Code[20]; Amount: Decimal) + begin + SalesHeader.Validate("Sell-to Customer No.", CustomerNo); + SalesLine.Insert(true); + + // Negative amounts arrive from credit memos routed through this + // codeunit; PostEntry() rejects them, so they're filtered here. + if Amount < 0 then + exit; + + if Amount > 0 then + PostEntry(Amount); + end; + + local procedure PostEntry(Amount: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md new file mode 100644 index 0000000..52c3c0a --- /dev/null +++ b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: style +keywords: [comments, verbosity, self-documenting, restate, tutorial-style, credit-memo-routing] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Comments must not restate what the code already shows + +## Description + +A comment above nearly every statement that just narrates what the statement already says (`// Validate the customer number` above `SalesHeader.Validate("Sell-to Customer No.", CustomerNo)`) adds noise without adding information. Production AL — the Base Application, mature partner codebases — is comment-sparse by comparison: identifiers do the explaining, and a comment appears only when the code alone can't carry the reason. + +A comment earns its place only when it captures something the code cannot: a non-obvious business rule, a workaround for a specific platform limitation, or a constraint that would surprise the next reader. If removing the comment would leave the reader no worse off, the comment should not have been written. + +This does not override required structural documentation — feature/scenario test tags and XML-doc summaries on public library procedures remain required where they apply; those are structural markers, not narrative comments. + +## Best Practice + +Let the code speak for itself; reserve comments for the reason a reader could not otherwise infer. + +See sample: [`al-comments-must-not-restate-what-code-already-shows.good.al`](al-comments-must-not-restate-what-code-already-shows.good.al). + +## Anti Pattern + +A comment line before every statement, repeating in English what the statement's own identifiers already say. + +See sample: [`al-comments-must-not-restate-what-code-already-shows.bad.al`](al-comments-must-not-restate-what-code-already-shows.bad.al). diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al b/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al new file mode 100644 index 0000000..352c515 --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al @@ -0,0 +1,49 @@ +table 50101 "Sample Order Line" +{ + fields + { + field(1; "Document No."; Code[20]) { } + field(2; "Line No."; Integer) { } + field(10; Quantity; Decimal) { } + field(11; "Unit Price"; Decimal) { } + field(12; "Line Amount"; Decimal) { } + } + keys + { + key(PK; "Document No.", "Line No.") { Clustered = true; } + } +} + +page 50100 "Sample Order Line Card" +{ + PageType = Card; + SourceTable = "Sample Order Line"; + + layout + { + area(content) + { + repeater(General) + { + field(quantity; Rec.Quantity) { } + field(unitPrice; Rec."Unit Price") { } + field(lineAmount; Rec."Line Amount") { } + } + } + } + + actions + { + area(Processing) + { + action(Recalculate) + { + trigger OnAction() + begin + Rec."Line Amount" := Rec.Quantity * Rec."Unit Price"; + Rec.Modify(); + end; + } + } + } +} diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al b/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al new file mode 100644 index 0000000..a22f307 --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al @@ -0,0 +1,60 @@ +table 50101 "Sample Order Line" +{ + fields + { + field(1; "Document No."; Code[20]) { } + field(2; "Line No."; Integer) { } + field(10; Quantity; Decimal) { } + field(11; "Unit Price"; Decimal) { } + field(12; "Line Amount"; Decimal) { } + } + keys + { + key(PK; "Document No.", "Line No.") { Clustered = true; } + } +} + +codeunit 50100 "Sample Order Line Management" +{ + procedure RecalculateLine(var OrderLine: Record "Sample Order Line") + begin + OrderLine.Validate("Line Amount", OrderLine.Quantity * OrderLine."Unit Price"); + OrderLine.Modify(true); + end; +} + +page 50100 "Sample Order Line Card" +{ + PageType = Card; + SourceTable = "Sample Order Line"; + + layout + { + area(content) + { + repeater(General) + { + field(quantity; Rec.Quantity) { } + field(unitPrice; Rec."Unit Price") { } + field(lineAmount; Rec."Line Amount") { } + } + } + } + + actions + { + area(Processing) + { + action(Recalculate) + { + trigger OnAction() + begin + OrderLineMgt.RecalculateLine(Rec); + end; + } + } + } + + var + OrderLineMgt: Codeunit "Sample Order Line Management"; +} diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.md b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md new file mode 100644 index 0000000..ed5a4cb --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md @@ -0,0 +1,31 @@ +--- +bc-version: [all] +domain: style +keywords: [pages, business-logic, codeunit, separation-of-concerns, presentation-layer, rec-modify] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep business logic out of page objects + +## Description + +A page procedure that persists a business mutation directly (`Rec.Modify()` outside the standard record-bound save, or a cross-entry-point business rule implemented only in a page trigger) is an architecture violation even when it compiles: the rule only applies when a user opens that specific page, and silently doesn't run through any other entry point (API, batch job, another page). This is narrower than "no calculation may live on a page" — a presentation-specific calculation (formatting, a derived display value) is fine on the page that shows it, and a reusable data invariant commonly belongs on the table itself (a field's own validation/trigger), not forced into a codeunit merely to keep it off the page. The actual line is entry-point independence: a business operation or invariant that must hold regardless of which entry point touches the record belongs in a codeunit or the table, not solely in one page's trigger. + +A narrow set of patterns are conventional rather than violations: +- A setup page reading and writing its own singleton setup record. +- A dedicated "Run Conversion" page invoking a conversion codeunit directly. +- The standard singleton-initialization idiom on `OnOpenPage` (`if not Rec.Get() then begin Rec.Init(); Rec.Insert(); end`) used by cue/activities pages to bootstrap their own presentation-state record — this is not business logic, it is the same pattern used throughout base-app cue pages. + +## Best Practice + +Delegate all business operations to a codeunit: the page owns presentation, the codeunit owns logic. A calculation or validation triggered from a page action should call a codeunit procedure rather than compute the result inline. + +See sample: [`pages-must-not-contain-business-logic.good.al`](pages-must-not-contain-business-logic.good.al). + +## Anti Pattern + +A cross-entry-point business rule or persisted mutation implemented only in a page trigger — calling `Rec.Modify()` to save a computed business value from `OnValidate`/`OnAction`, or a validation that must hold regardless of caller, instead of routed through a codeunit or the table's own field validation. A presentation-only calculation or a table-owned field invariant is not an instance of this anti-pattern. + +See sample: [`pages-must-not-contain-business-logic.bad.al`](pages-must-not-contain-business-logic.bad.al). diff --git a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md new file mode 100644 index 0000000..c698093 --- /dev/null +++ b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md @@ -0,0 +1,48 @@ +--- +bc-version: [all] +domain: style +keywords: [folder-structure, feature-organization, source-layout, maintainability] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Organize AL source by business feature, not object type + +## Description + +Folder structure inside an AL app has no effect on compilation or runtime behavior — this is a repository-organization convention, not a platform requirement, and different projects reasonably choose differently. Grouping files by business feature or module (`src/Sales/Invoice/`, `src/NoSeries/`) rather than by AL object type (`src/Tables/`, `src/Pages/`, `src/Codeunits/`) keeps everything belonging to one feature physically together, which many teams find easier to navigate than jumping between object-type folders that share nothing but their AL object kind. Adopt this consistently on a project rather than mixing both schemes, but treat it as a team convention to apply deliberately, not a Microsoft-mandated structure. + +Code genuinely shared across multiple features (utility codeunits, common interfaces, shared enums) belongs in a `Common` or `Shared` folder, not duplicated per feature and not left in a catch-all root. + +## Best Practice + + src/ + ├── NoSeries/ + ├── Sales/ + │ ├── Invoice/ + │ └── Order/ + └── Common/ + +Each feature folder holds every object type it needs; shared code has one dedicated home. + +## Anti Pattern + +A repository that documents or has established feature-based organization +as its convention, but then mixes in object-type folders for new work +anyway: + + src/ + ├── Sales/ + │ └── Invoice/ + ├── Tables/ <- new objects land here instead of a feature folder + └── Codeunits/ + +The anti-pattern is inconsistency with the project's own chosen convention, +not the object-type scheme itself — a repository that deliberately and +consistently organizes by object type throughout is exercising the other +reasonable choice described above, not violating this rule. What actually +costs a reader time is a codebase where some features live under their own +folder and others are scattered across type folders, so finding everything +related to one feature means checking both schemes and reassembling it from +wherever each object happened to land. diff --git a/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md b/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md index 4560a18..9ebf3f8 100644 --- a/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md +++ b/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md @@ -23,4 +23,6 @@ See sample: [`asserterror-needs-expectederror-and-code.good.al`](asserterror-nee `asserterror DoInvalid();` with nothing after it. The test asserts only that the call failed somehow; swap the validation for a different bug and the test still passes, certifying a guard that may no longer fire. A negative test that cannot tell one error from another verifies almost nothing. +Not an instance of this anti-pattern: a trailing `asserterror Error(SomeLabel)` used purely as an end-of-test rollback sentinel to undo a lazily-initialized shared fixture's scratch changes (see `commit-shared-test-fixture-inside-lazy-initialize.md`). That `Error` call exists to force a rollback, not to verify that a specific failure occurred — the sentinel's own text is not meant to be asserted against, and adding an `ExpectedError` there would just duplicate the label without checking anything the test doesn't already control. + See sample: [`asserterror-needs-expectederror-and-code.bad.al`](asserterror-needs-expectederror-and-code.bad.al). diff --git a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al new file mode 100644 index 0000000..d6a29d6 --- /dev/null +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al @@ -0,0 +1,29 @@ +// Only wraps Microsoft's own generic scenario — measures BC, not this extension +codeunit 50101 "BCPT Create Sales Order" implements "BCPT Test Param. Provider" +{ + SingleInstance = true; + + trigger OnRun() + begin + CreateStandardSalesOrder(GlobalBCPTTestContext); + end; + + var + GlobalBCPTTestContext: Codeunit "BCPT Test Context"; + + local procedure CreateStandardSalesOrder(var BCPTTestContext: Codeunit "BCPT Test Context") + begin + BCPTTestContext.StartScenario('Create Sales Order With N Lines'); + // ... standard sales order creation, no reference to the extension's own logic + BCPTTestContext.EndScenario('Create Sales Order With N Lines'); + end; + + procedure GetDefaultParameters(): Text[1000] + begin + exit(''); + end; + + procedure ValidateParameters(Parameters: Text[1000]) + begin + end; +} diff --git a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al new file mode 100644 index 0000000..68239d7 --- /dev/null +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al @@ -0,0 +1,73 @@ +codeunit 50100 "BCPT Create Service Request" implements "BCPT Test Param. Provider" +{ + SingleInstance = true; + + trigger OnRun() + begin + if not IsInitialized then begin + InitTest(); + IsInitialized := true; + end; + CreateServiceRequest(GlobalBCPTTestContext); + end; + + var + GlobalBCPTTestContext: Codeunit "BCPT Test Context"; + CustomerNo: Code[20]; + IsInitialized: Boolean; + + local procedure InitTest() + var + Customer: Record Customer; + begin + // Do not assume a customer already exists: a BCPT run may target an + // otherwise-empty environment. Create one if none is found instead + // of failing on FindFirst(). + if not Customer.FindFirst() then begin + Customer.Init(); + Customer."No." := GenerateUniqueCode(MaxStrLen(Customer."No.")); + Customer.Insert(true); + end; + CustomerNo := Customer."No."; + end; + + local procedure GenerateUniqueCode(Length: Integer): Code[20] + begin + // A GUID-derived code, not a session-local counter: it stays unique + // across concurrent BCPT sessions and repeated runs against the + // same environment, which an in-memory counter reset per session + // cannot guarantee. + exit(CopyStr(DelChr(Format(CreateGuid()), '=', '{}-'), 1, Length)); + end; + + local procedure CreateServiceRequest(var BCPTTestContext: Codeunit "BCPT Test Context") + var + ServiceRequestHeader: Record "Service Request Header"; + ServiceRequestLine: Record "Service Request Line"; + begin + BCPTTestContext.StartScenario('Create Service Request Header'); + ServiceRequestHeader.Init(); + ServiceRequestHeader."No." := GenerateUniqueCode(MaxStrLen(ServiceRequestHeader."No.")); + ServiceRequestHeader.Validate("Customer No.", CustomerNo); + ServiceRequestHeader.Insert(true); + BCPTTestContext.EndScenario('Create Service Request Header'); + BCPTTestContext.UserWait(); + + BCPTTestContext.StartScenario('Add Service Request Line'); + ServiceRequestLine.Init(); + ServiceRequestLine."Document No." := ServiceRequestHeader."No."; + ServiceRequestLine."Line No." := 10000; + ServiceRequestLine.Description := 'Performance test line'; + ServiceRequestLine.Insert(true); + BCPTTestContext.EndScenario('Add Service Request Line'); + end; + + procedure GetDefaultParameters(): Text[1000] + begin + exit(''); + end; + + procedure ValidateParameters(Parameters: Text[1000]) + begin + end; +} diff --git a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md new file mode 100644 index 0000000..51ebddd --- /dev/null +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [bcpt, performance-test, scenarios, app-specific, regression] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Include app-specific scenarios in a PerformanceTest app's BCPT suite + +## Description + +A PerformanceTest app that ships with only the generic Microsoft BCPT samples (creating sales orders, purchase orders, posting item journals) measures Business Central's own baseline performance, not the extension it was built to test. Those samples are starting points, not coverage. Without a scenario that exercises the extension's own business flow — its own codeunits, its own FlowFields, its own page rendering — a performance regression introduced by the extension has no test that would ever detect it. + +## Best Practice + +For every major business flow the extension adds, create a matching `BCPT*` scenario codeunit implementing `"BCPT Test Param. Provider"`, building its own test data in a local `InitTest()` procedure rather than depending on hardcoded records. Beyond that shared shape, the interface details are context-dependent, not fixed requirements: most of Microsoft's own shipped BCPT samples declare `SingleInstance = true`, but `codeunit "BCPT Create Customer"` does not, relying instead on `OnRun` calling `InitTest()` unconditionally every run. Likewise, wrapping the operation under test in `BCPTTestContext.StartScenario()` / `EndScenario()` is a real, available pattern for splitting one codeunit's run into several separately measured steps — useful when a regression in one step should not hide inside a coarser, whole-`OnRun` measurement — but it is not what every sample does; `"BCPT Create Customer"` measures its entire `OnRun` as a single implicit scenario and never calls `StartScenario`/`EndScenario` at all. Choose per-step scenarios when step-level granularity matters to the flow being tested; otherwise a single measured `OnRun` is a legitimate, simpler choice. + +See sample: [`bcpt-scenarios-must-be-app-specific.good.al`](bcpt-scenarios-must-be-app-specific.good.al). + +## Anti Pattern + +A PerformanceTest app whose only scenario codeunits are copies of Microsoft's shipped samples (creating a standard sales order, opening the standard customer list) tests the platform, not the extension. Any regression in the extension's own posting logic, calculations, or pages goes unmeasured and unnoticed. + +See sample: [`bcpt-scenarios-must-be-app-specific.bad.al`](bcpt-scenarios-must-be-app-specific.bad.al). diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al new file mode 100644 index 0000000..f2af637 --- /dev/null +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al @@ -0,0 +1,73 @@ +codeunit 50142 "Sample Test Library" +{ + Subtype = Test; + + var + LibraryInventory: Codeunit "Library - Inventory"; + Initialized: Boolean; + SharedItemNo: Code[20]; + RollBackMsg: Label 'Revert back the tables to their original state.'; + + local procedure Initialize() + begin + if Initialized then + exit; + + CreateSharedFixtureData(); + // BUG: no Commit() here. The fixture below is still inside this + // test method's own transaction. + Initialized := true; + end; + + local procedure CreateSharedFixtureData() + var + Item: Record Item; + begin + LibraryInventory.CreateItem(Item); + SharedItemNo := Item."No."; + end; + + [Test] + procedure FirstTestUsesSharedFixture() + var + Item: Record Item; + begin + Initialize(); + + Item.Get(SharedItemNo); + Item.Description := 'Scratch change this test makes and does not need to keep.'; + Item.Modify(); + + asserterror Error(RollBackMsg); + // The deliberate rollback above also erases the never-committed + // fixture from CreateSharedFixtureData(). Initialized still reads + // true on the next test, but the row it points at is gone. + end; + + [Test] + procedure SecondTestStillFindsSharedFixture() + var + Item: Record Item; + begin + Initialize(); + + // Fails here: Initialize() saw Initialized = true and returned + // immediately, so it never recreated the fixture - and the first + // test's rollback took the original row with it. + Item.Get(SharedItemNo); + end; +} + +codeunit 50143 "Sample Test Runner" +{ + // Codeunit isolation alone does not save this fixture: TestIsolation + // only controls whether committed changes survive between methods, and + // this fixture was never committed in the first place. + Subtype = TestRunner; + TestIsolation = Codeunit; + + trigger OnRun() + begin + Codeunit.Run(Codeunit::"Sample Test Library"); + end; +} diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al new file mode 100644 index 0000000..c550697 --- /dev/null +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al @@ -0,0 +1,71 @@ +codeunit 50142 "Sample Test Library" +{ + Subtype = Test; + + var + LibraryInventory: Codeunit "Library - Inventory"; + Initialized: Boolean; + SharedItemNo: Code[20]; + RollBackMsg: Label 'Revert back the tables to their original state.'; + + local procedure Initialize() + begin + if Initialized then + exit; + + CreateSharedFixtureData(); + Commit(); + Initialized := true; + end; + + local procedure CreateSharedFixtureData() + var + Item: Record Item; + begin + LibraryInventory.CreateItem(Item); + SharedItemNo := Item."No."; + end; + + [Test] + procedure FirstTestUsesSharedFixture() + var + Item: Record Item; + begin + Initialize(); + + Item.Get(SharedItemNo); + Item.Description := 'Scratch change this test makes and does not need to keep.'; + Item.Modify(); + + asserterror Error(RollBackMsg); + // Rolls back the Modify() above, but not the fixture: that was + // already committed inside Initialize(). + end; + + [Test] + procedure SecondTestStillFindsSharedFixture() + var + Item: Record Item; + begin + // Runs after FirstTestUsesSharedFixture's deliberate rollback. + // Initialize() sees Initialized = true and does nothing, but the + // committed fixture it created earlier is still there to Get(). + Initialize(); + + Item.Get(SharedItemNo); + end; +} + +codeunit 50143 "Sample Test Runner" +{ + // Codeunit isolation: everything this codeunit's tests commit, + // including the shared fixture, survives from one test method to the + // next, and rolls back only once every method in the codeunit has run. + Subtype = TestRunner; + TestIsolation = Codeunit; + + trigger OnRun() + begin + Codeunit.Run(Codeunit::"Sample Test Library"); + end; +} diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md new file mode 100644 index 0000000..7ec16df --- /dev/null +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md @@ -0,0 +1,34 @@ +--- +bc-version: [all] +domain: testing +keywords: [initialize, isinitialized, shared-fixture, commit, autocommit, asserterror, testisolation, lazy-initialization] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Commit shared fixture data created inside a lazy Initialize(), or later tests lose it + +## Description + +A test method with no `[TransactionModel(...)]` attribute defaults to `AutoCommit` (see `transactionmodel-attribute-governs-test-transactions.md`): a method that completes without error commits automatically at its own boundary, with no explicit `Commit()` needed. So a lazy/shared `Initialize()` — guarded by an `IsInitialized` flag, creating master/setup data once to avoid repeating expensive setup across many `[Test]` methods — does not need `Commit()` just to survive into the next test method; under the default model it already will. (Declaring `[TransactionModel(AutoRollback)]` instead is not compatible with this pattern at all: `AutoRollback` assumes the code under test never commits, and a `Commit()` call under it raises a runtime error.) + +What an early `Commit()` inside `Initialize()` actually guards against is the test method's *own later, deliberate* rollback — the BCApps cleanup idiom of ending a test with `asserterror Error(SomeLabel)` to undo demo-data mutations that method made, so the run doesn't permanently dirty the database. Per the documented `Codeunit.Run` transaction semantics, changes are committed at the end of an execution "unless an error occurs" — an unhandled error rolls back whatever wasn't already committed. `Commit()` closes out the fixture's own transaction immediately, so it is unaffected by whatever the rest of that method does afterward, including that end-of-test error. Without the early `Commit()`, the same deliberate rollback wipes out the fixture too, even though `IsInitialized` still reads `true` on the next test, since it's a plain variable, not persisted data. BCApps' `codeunit 134915 "ERM Online Mapping Setup"` shows exactly this shape: no `TransactionModel` attribute, `Commit()` inside a lazy `Initialize()`, and the test itself ends with `asserterror Error(RollBackMessage)`. + +Protecting the fixture from that same-method rollback is necessary but not sufficient for the fixture to reach a *later* test method — that also depends on the executing test runner's `TestIsolation`. Under `Disabled` (the property's own documented default) or `Codeunit` (used by BCApps' own `TestRunner`, `CLITestRunner`, and `SnapTestRunner` codeunits), nothing rolls back until the whole test codeunit finishes, so the already-committed fixture survives across every method run before then. These two are not interchangeable, though: `Codeunit` rolls back everything once the codeunit's last method completes, so the environment is clean afterward; `Disabled` never rolls back anything at all — "tests are not isolated from each other" is the property's own description — so a fixture this pattern commits stays in the database permanently unless something else explicitly deletes it. Under `Function`, the runner rolls back all database changes — explicitly including ones already committed via `Commit()` — after every single test method; no amount of committing inside `Initialize()` makes a fixture shared across methods survive that regime, because the whole premise of a lazy, once-per-codeunit fixture doesn't hold when every method is isolated from every other. + +## Best Practice + +When a test method's own cleanup relies on ending in a deliberate error to roll back its scratch changes, call `Commit()` once, inside the lazy `Initialize()` guard, right after the shared fixture is created — before that cleanup-triggering error can run. This pattern only delivers a fixture shared across test methods when the executing runner's `TestIsolation` is `Disabled` or `Codeunit`; do not recommend it, or pair it with, a `Function`-isolated runner — that configuration undoes the committed fixture after every method regardless. Recommend `TestIsolation = Codeunit`: it gives every method in the codeunit the same shared, committed fixture and still leaves the database clean once the codeunit finishes. Recommend `Disabled` only alongside an explicit, verified teardown step that removes the fixture data at the end of the run — without one, the committed fixture is permanent contamination, not a controlled trade-off. + +See sample: [`commit-shared-test-fixture-inside-lazy-initialize.good.al`](commit-shared-test-fixture-inside-lazy-initialize.good.al). + +## Anti Pattern + +A shared `Initialize()` guarded by `IsInitialized` that creates fixture records without committing, in a test method that ends with a deliberate `asserterror Error(...)` to undo its own scratch changes, run under a `Disabled`- or `Codeunit`-isolated test runner. That rollback also erases the never-committed fixture; the next test still finds `IsInitialized = true` but the rows it depends on are gone. (Under a `Function`-isolated runner the fixture is lost regardless of `Commit()`, for the unrelated reason above — that is a runner-configuration problem, not this anti-pattern.) + +See sample: [`commit-shared-test-fixture-inside-lazy-initialize.bad.al`](commit-shared-test-fixture-inside-lazy-initialize.bad.al). + +## Source + +The shared/lazy `Initialize()` pattern and its `Commit()` call are drawn from Luc van Vugt's "Let's talk about Shared Fixture and how to profit from this with the Dynamics NAV Test Toolkit": https://www.fluxxus.nl/index.php/bc/let39s-talk-about-shared-fixture-and-how-to-profit-from-this-with-the-dynamics-nav-test-toolkit/. That post shows the `Commit()` call in its `Initialize()` example but does not explain the transaction mechanics behind it; the `AutoCommit`-default, `Codeunit.Run`-error, and `TestIsolation`-level analysis above is this article's own, verified independently against Microsoft's TransactionModel/TestIsolation documentation and BCApps' `codeunit 134915 "ERM Online Mapping Setup"` source, not taken from the post. diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al new file mode 100644 index 0000000..0181fdb --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al @@ -0,0 +1,15 @@ +[Test] +procedure PostSalesOrder_CreatesInvoice() +var + SalesHeader: Record "Sales Header"; + SalesInvoiceHeader: Record "Sales Invoice Header"; + InvoiceNo: Code[20]; +begin + // [GIVEN] a sales order — posting groups left to whatever exists in the test company + LibrarySales.CreateSalesOrder(SalesHeader); + // [WHEN] + InvoiceNo := LibrarySales.PostSalesDocument(SalesHeader, false, true); + // [THEN] + SalesInvoiceHeader.Get(InvoiceNo); + Assert.RecordIsNotEmpty(SalesInvoiceHeader); +end; diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al new file mode 100644 index 0000000..224f0dc --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al @@ -0,0 +1,20 @@ +[Test] +procedure PostSalesOrder_CreatesInvoice() +var + Customer: Record Customer; + SalesHeader: Record "Sales Header"; + SalesInvoiceHeader: Record "Sales Invoice Header"; + InvoiceNo: Code[20]; +begin + // [GIVEN] a customer + LibrarySales.CreateCustomer(Customer); + // [GIVEN] a sales order for that customer + LibrarySales.CreateSalesOrderForCustomerNo(SalesHeader, Customer."No."); + SalesHeader.Validate("Posting Date", WorkDate()); + SalesHeader.Modify(true); + // [WHEN] + InvoiceNo := LibrarySales.PostSalesDocument(SalesHeader, false, true); + // [THEN] + SalesInvoiceHeader.Get(InvoiceNo); + Assert.RecordIsNotEmpty(SalesInvoiceHeader); +end; diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md new file mode 100644 index 0000000..b34adfe --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [given, test-setup, posting, report, request-page, precondition, completeness] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Cover the full precondition chain in GIVEN, not just the primary record + +## Description + +A `[GIVEN]` block is only correct if it sets up every precondition the code under test actually reads, not just the record the scenario is "about." For most master-data tests, creating the primary record is enough. For posting routines and reports it usually is not: an incomplete `[GIVEN]` produces a test that either fails with a setup error unrelated to the scenario, or worse, passes without ever reaching the logic it claims to verify. + +## Best Practice + +For a posting test, set up the full posting-group chain the document requires (e.g. customer/vendor posting group, gen. business/product posting group, VAT posting setup), the setup records the specific posting path reads, and an explicit date when the path is date-sensitive — a missing link surfaces as an unrelated G/L error, not a meaningful test failure. For a report test that claims to verify filtering or dataset logic, include both a record that should be included and one that should be excluded, plus any request-page parameter or FlowField the report's logic branches on. A report test that only claims to run without error is exempt from the include/exclude pairing, but it must say so in its scenario name or comment — an unlabelled single-record `[GIVEN]` is ambiguous about which claim it is making, and that ambiguity is itself the defect. + +See sample: [`given-blocks-must-cover-full-precondition-chain.good.al`](given-blocks-must-cover-full-precondition-chain.good.al). + +## Anti Pattern + +A posting test whose `[GIVEN]` creates only the sales header, relying on whatever posting groups happen to exist in the test company. A report test whose `[GIVEN]` creates only matching records, so the report "passes" whether or not its filter logic does anything at all. + +See sample: [`given-blocks-must-cover-full-precondition-chain.bad.al`](given-blocks-must-cover-full-precondition-chain.bad.al). diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al new file mode 100644 index 0000000..d95d9f2 --- /dev/null +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al @@ -0,0 +1,47 @@ +table 50144 "Sample Setup" +{ + fields + { + field(1; "Primary Key"; Code[10]) { } + field(2; "Default Category Code"; Code[20]) { } + } + keys + { + key(PK; "Primary Key") { Clustered = true; } + } +} + +table 50145 "Sample Header" +{ + fields + { + field(1; "No."; Code[20]) { } + field(10; "Category Code"; Code[10]) + { + TableRelation = "Sample Setup"."Primary Key"; + } + field(11; "Parent No."; Code[20]) + { + TableRelation = "Sample Header"."No."; + } + } + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +codeunit 50141 "Sample Table Relation Test Ext" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Table Relation Test", 'OnAfterRemoveTableRelation', '', false, false)] + local procedure ExcludeSampleFieldFromTableRelationTest(var TableRelationsMetadata: Record "Table Relations Metadata" temporary) + var + TableRelationTest: Codeunit "Table Relation Test"; + begin + // Removes every relation on the whole table (field/related table/ + // related field all 0), not just the one known exception - this + // also strips "Parent No." -> "Sample Header"."No.", which had no + // exception and should have stayed covered by the standard test. + TableRelationTest.RemoveTableRelation(TableRelationsMetadata, Database::"Sample Header", 0, 0, 0); + end; +} diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al new file mode 100644 index 0000000..26db161 --- /dev/null +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al @@ -0,0 +1,51 @@ +table 50144 "Sample Setup" +{ + fields + { + field(1; "Primary Key"; Code[10]) { } + field(2; "Default Category Code"; Code[20]) { } + } + keys + { + key(PK; "Primary Key") { Clustered = true; } + } +} + +table 50145 "Sample Header" +{ + fields + { + field(1; "No."; Code[20]) { } + // A known exception: "Category Code" predates "Sample Setup" and + // can carry a value that no longer resolves to a real row there, + // so the standard Table Relation Test would otherwise reject it - + // excluded via OnAfterRemoveTableRelation below. + field(10; "Category Code"; Code[10]) + { + TableRelation = "Sample Setup"."Primary Key"; + } + // An ordinary relation with no exception - ExcludeSampleFieldFrom + // TableRelationTest below must leave this one checked. + field(11; "Parent No."; Code[20]) + { + TableRelation = "Sample Header"."No."; + } + } + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +codeunit 50141 "Sample Table Relation Test Ext" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Table Relation Test", 'OnAfterRemoveTableRelation', '', false, false)] + local procedure ExcludeSampleFieldFromTableRelationTest(var TableRelationsMetadata: Record "Table Relations Metadata" temporary) + var + TableRelationTest: Codeunit "Table Relation Test"; + begin + // Removes only the one known exception. "Parent No." -> "Sample + // Header"."No." is untouched and stays covered by the standard test. + TableRelationTest.RemoveTableRelation(TableRelationsMetadata, Database::"Sample Header", 10, Database::"Sample Setup", 1); + end; +} diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md new file mode 100644 index 0000000..3f7d778 --- /dev/null +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md @@ -0,0 +1,35 @@ +--- +bc-version: [all] +domain: testing +keywords: [table-relation-test, tablerelationsmetadata, onafterremovetablerelation, field-length, field-type] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Exclude a known-valid TableRelation exception via OnAfterRemoveTableRelation + +## Description + +Codeunit 134926 "Table Relation Test" (shipped in BCApps' test app — only consumers that depend on the BC test libraries can subscribe to it) reads Table Relations Metadata tenant-wide across every installed app, not just the current one, and validates each field's type and length against what its relations require — but the exact rule depends on whether that field has an *unconditional* relation (a `Table Relations Metadata` row with `Condition Field No. = 0`) among its relations, or only *conditional* ones: + +- If any relation is unconditional, the field's length must equal *exactly* the largest related field's length, and its type must exactly match the required type — resolved to `Text` when the related fields themselves mix `Code` and `Text`. +- If every relation for that field is conditional, the requirement relaxes: the field only needs to be *at least* as long as the largest related field (longer is accepted; only shorter fails), and when the required type is specifically `Code`, both a `Code` and a `Text` source field pass. That `Code`/`Text` tolerance is conditional-only — it does not apply on the unconditional side, and it does not extend to a required type of `Text` (a `Code` source field does not satisfy a required `Text`). + +A field with a legitimate, intentional relation shape outside both of these tolerances has no per-field override in its own object definition; the check runs with no built-in escape hatch beyond them. The validation test method itself is `[Scope('OnPrem')]`: it only runs from an on-premises test surface, not from a cloud-targeted test app, so this whole exception mechanism — and the check it works around — is only reachable where that test can actually execute. + +## Best Practice + +Subscribe to `OnAfterRemoveTableRelation` and call the codeunit's own `RemoveTableRelation(TableRelationsMetadata, TableID, FieldID, RelatedTableID, RelatedFieldID)` to strike the one known-valid relation before the test evaluates it, scoped as narrowly as the exception actually is. Because the test itself is `[Scope('OnPrem')]`, do not recommend subscribing to it as a way to guard a cloud-targeted app's test suite — the subscription has no effect where the test never runs. + +See sample: [`table-relation-test-exclude-known-invalid-relations-via-event.good.al`](table-relation-test-exclude-known-invalid-relations-via-event.good.al). + +## Anti Pattern + +Excluding an entire table's relations (or disabling the whole test codeunit) to work around one known exception. This discards the check's coverage for every other relation on that table, or in the app, not just the one that needed an exception. + +See sample: [`table-relation-test-exclude-known-invalid-relations-via-event.bad.al`](table-relation-test-exclude-known-invalid-relations-via-event.bad.al). + +## Source + +The `OnAfterRemoveTableRelation` exclusion technique is drawn from Luc van Vugt's "How-to: Test your Table Relations (2)": https://www.fluxxus.nl/index.php/bc/how-to-test-your-table-relations-2/. The codeunit/event signature, the `[Scope('OnPrem')]` boundary, and the tenant-wide `Table Relations Metadata` scope described above were verified directly against BCApps' `codeunit 134926 "Table Relation Test"` source, not taken from the post. diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al b/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al new file mode 100644 index 0000000..06e5d54 --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al @@ -0,0 +1,33 @@ +codeunit 50102 "Item Price Testing" +{ + Subtype = Test; + + var + LibrarySales: Codeunit "Library - Sales"; + LibraryInventory: Codeunit "Library - Inventory"; + LibraryPriceCalculation: Codeunit "Library - Price Calculation"; + Assert: Codeunit "Library Assert"; + + [Test] + procedure Test1() + var + Customer: Record Customer; + Item: Record Item; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; + begin + // setup mixed with assertions, no clear layers, no FEATURE/SCENARIO/GIVEN/WHEN/THEN tags + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", ''); + end; +} diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al new file mode 100644 index 0000000..b2cbf06 --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al @@ -0,0 +1,40 @@ +// [FEATURE] Item Price — price cascade (Customer -> Price Group -> All Customers) +codeunit 50103 "Item Price Testing" +{ + Subtype = Test; + + var + LibrarySales: Codeunit "Library - Sales"; + LibraryInventory: Codeunit "Library - Inventory"; + LibraryPriceCalculation: Codeunit "Library - Price Calculation"; + Assert: Codeunit "Library Assert"; + + [Test] + procedure GetPrice_CustomerPrice_ReturnsUnitPrice() + var + Customer: Record Customer; + Item: Record Item; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; + begin + // [SCENARIO] Customer with a specific price list line gets that unit price + // [GIVEN] a customer and an item with a customer-specific sales price list line + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + // CreatePriceHeader leaves the list in Draft status, which price calculation ignores. + PriceListHeader.Validate(Status, PriceListHeader.Status::Active); + PriceListHeader.Modify(true); + // [WHEN] a sales line is created for that customer and item + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); + // [THEN] the sales line picks up the customer's price list line + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", 'Unit price must match customer price list'); + end; +} diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.md b/microsoft/knowledge/testing/test-feature-scenario-tags.md new file mode 100644 index 0000000..89198fb --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [feature, scenario, given, when, then, tags, bdd, atdd, comments, subtype-test] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Tag test codeunits with FEATURE, SCENARIO, GIVEN, WHEN, and THEN comments + +## Description + +Test codeunits are easier to trust and to review when they carry a four-level comment structure taken from Behaviour-/Acceptance-Test-Driven Development: `[FEATURE]` once at the top of the codeunit naming the functional area under test, `[SCENARIO]` above each test procedure stating one falsifiable business claim in plain language, and `[GIVEN]`/`[WHEN]`/`[THEN]` marking the precondition, action, and assertion inside the test body. Without these tags a test procedure is an opaque block of AL that only reveals its intent by being read line by line; a reviewer or product owner cannot scan a codeunit and know what business behaviour it covers. + +## Best Practice + +Put `[FEATURE]` as a comment before the codeunit's opening brace, naming the domain rather than the object — Microsoft's own guidance allows setting it once for the whole codeunit, inherited by every test in it. Put `[SCENARIO]`, matching the current BCApps corpus, as the first comment inside each test procedure's body (after `begin`), describing the scenario in business language that complements — not duplicates — the procedure name, followed by `[GIVEN]` marking the precondition setup, `[WHEN]` marking the single action under test, and `[THEN]` marking the assertions. The procedure name stays the machine-readable identity shown in test-runner output; the `[SCENARIO]` comment stays the human-readable one. Neither replaces the other. + +See sample: [`test-feature-scenario-tags.good.al`](test-feature-scenario-tags.good.al). + +## Anti Pattern + +A test procedure with no `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` structure, setup mixed freely with assertions, and a procedure name like `Test1` that says nothing about what is being verified. Nothing in the codeunit tells a reader what business rule it exists to protect. + +See sample: [`test-feature-scenario-tags.bad.al`](test-feature-scenario-tags.bad.al). diff --git a/microsoft/knowledge/testing/test-one-when-per-test.bad.al b/microsoft/knowledge/testing/test-one-when-per-test.bad.al new file mode 100644 index 0000000..c49696c --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.bad.al @@ -0,0 +1,32 @@ +[Test] +procedure GetPrice_ThenGetPriceLines_ReturnsCorrectValues() +var + Customer: Record Customer; + Item: Record Item; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; +begin + // [GIVEN] ... + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + // [WHEN] first action + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); + // [WHEN] second action — this is a second test in disguise + PriceListLine.Validate("Minimum Quantity", 10); + PriceListLine.Modify(true); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + // [THEN] asserting two unrelated things + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", ''); + PriceListLine.SetRange("Price List Code", PriceListHeader.Code); + Assert.AreEqual(2, PriceListLine.Count(), ''); +end; diff --git a/microsoft/knowledge/testing/test-one-when-per-test.good.al b/microsoft/knowledge/testing/test-one-when-per-test.good.al new file mode 100644 index 0000000..6b58c51 --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.good.al @@ -0,0 +1,56 @@ +[Test] +procedure GetPrice_CustomerPrice_ReturnsCorrectUnitPrice() +var + Customer: Record Customer; + Item: Record Item; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; +begin + // [GIVEN] a customer with a price list line for the item + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + // CreatePriceHeader leaves the list in Draft status, which price calculation ignores. + PriceListHeader.Validate(Status, PriceListHeader.Status::Active); + PriceListHeader.Modify(true); + // [WHEN] + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); + // [THEN] + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", 'Unit price must match price list'); +end; + +[Test] +procedure GetPriceLines_TwoMinimumQuantityLines_ReturnsBoth() +var + Customer: Record Customer; + Item: Record Item; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; +begin + // [GIVEN] a customer price list with two minimum-quantity price lines for the same item + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + PriceListLine.Validate("Minimum Quantity", 10); + PriceListLine.Modify(true); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + PriceListLine.Validate("Minimum Quantity", 50); + PriceListLine.Modify(true); + // [WHEN] + PriceListLine.SetRange("Price List Code", PriceListHeader.Code); + // [THEN] + Assert.AreEqual(2, PriceListLine.Count(), 'Exactly two price lines expected'); +end; diff --git a/microsoft/knowledge/testing/test-one-when-per-test.md b/microsoft/knowledge/testing/test-one-when-per-test.md new file mode 100644 index 0000000..69c3b37 --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.md @@ -0,0 +1,34 @@ +--- +bc-version: [all] +domain: testing +keywords: [when, single-action, bdd, atdd, given-when-then, flow-test, regression-test] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep exactly one WHEN per test, with narrow exceptions for flow and defect-then-fix tests + +## Description + +This is a testing-design practice, not a BC platform requirement — no AL API enforces it, and it should not gate a change the way a platform-contradicted claim would. Each test procedure should contain exactly one `[WHEN]` block: one action that triggers the behaviour under test. A test with multiple WHENs — "do A, then do B, then check C" — is two or more tests in disguise. Splitting them gives failure isolation (a failing test points at one action, not an ambiguous sequence) and keeps each test readable as a single, falsifiable claim. A precondition action, such as posting a document so a ledger entry exists to assert against, belongs in `[GIVEN]`; only the action actually being asserted belongs in `[WHEN]`. + +## Best Practice + +Give each test one `[WHEN]` and one focused claim. A procedure name containing "And" or "Then" in the middle (`GetPrice_AndDiscount_ReturnsValues`) is a strong signal the test should be split. + +See sample: [`test-one-when-per-test.good.al`](test-one-when-per-test.good.al). + +## Anti Pattern + +A test that performs a first action, then a second unrelated action, then asserts on both — mixing two falsifiable claims into one procedure so a failure can't tell you which action broke. + +See sample: [`test-one-when-per-test.bad.al`](test-one-when-per-test.bad.al). + +## Flow tests — a deliberate exception + +A flow test verifies the accumulated outcome of a genuinely multi-round business process (partial receipt then invoicing, several posting rounds against one document), where the sequence itself is the scenario — splitting it would lose the interaction under test. Multiple `[WHEN]` blocks are allowed only when the procedure name declares the flow, each `[WHEN]` is labelled as one round of a single scenario rather than an unrelated action, and the `[THEN]` asserts the accumulated end-state rather than assertions that decompose cleanly per action (if they do decompose cleanly, it is still two tests in disguise). Outside this shape, unit-level tests keep the strict one-WHEN rule. + +## Defect-then-fix tests — a second, narrower exception + +A test that reproduces a specific broken state and then verifies a subsequent action corrects it is not the same shape as an unrelated-action test, even though its `[THEN]` assertions decompose cleanly per step — clean decomposition is expected here, not a sign of two unrelated tests. This shape is permitted only when the second `[WHEN]` cannot be meaningfully tested without the first (the fix only affects the exact stale state the first action produced, so splitting would just re-run the first action inside a second test's `[GIVEN]`), and the procedure name communicates the before/after relationship. diff --git a/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md b/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md index c9c159a..0f4f3bd 100644 --- a/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md +++ b/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md @@ -11,16 +11,20 @@ application-area: [all] ## Description -`[TransactionModel(...)]` declares how a test method interacts with the database's write transaction. The attribute applies only to methods inside a codeunit with `SubType = Test` and takes one of three values: `AutoRollback`, `AutoCommit`, or `None`. The choice must match the code being exercised — in particular, whether that code calls `Commit()`. Per the platform reference, "if the code that you test includes calls to the COMMIT Method, then set the TransactionModel property on the test method to AutoCommit." Applying `AutoRollback` to a test that drives code which calls `Commit` produces a runtime error on the first Commit, not a meaningful assertion failure — the test does not complete, and the reviewer sees an infrastructure error instead of a business-logic verdict. +`[TransactionModel(...)]` declares how a test method interacts with the database's write transaction. The attribute applies only to methods inside a codeunit with `SubType = Test` and takes one of three values: `AutoRollback`, `AutoCommit`, or `None`. **`AutoCommit` is the documented default** — a test method with no `[TransactionModel(...)]` attribute at all runs under `AutoCommit`, not `AutoRollback` and not `None` (Microsoft's TransactionModel property reference states this explicitly: "AutoCommit is the default value"). The "a call to `Commit` produces a runtime error" behavior is specific to the *explicitly declared* `AutoRollback` attribute. BCApps' own canonical pattern for a lazily-initialized shared fixture (see `codeunit 134915 "ERM Online Mapping Setup"`) declares no `TransactionModel` attribute at all — so it runs under the `AutoCommit` default — calls `Commit()` inside its `Initialize()` helper, and cleans up manually with a deliberate `asserterror Error(...)` at the end rather than relying on automatic rollback; this is a legitimate, common pattern, not a bug. Per the same reference, under `AutoCommit` an error, even one caught by `asserterror`, still rolls back the transaction — but "only to the point at which `Commit` was called" if the code being tested committed first. When a test method *does* declare `AutoRollback` explicitly, the choice must match the code being exercised: per the platform reference, "if the code that you test includes calls to the COMMIT Method, then set the TransactionModel property on the test method to AutoCommit." Applying `AutoRollback` to a test that drives code which calls `Commit` produces a runtime error on the first Commit, not a meaningful assertion failure. ## Best Practice -Default to `AutoRollback`: it opens a write transaction at the start of the test, runs the test body, and rolls back at the end, leaving the database in its original state. Pick `AutoCommit` only when the code under test genuinely calls `Commit` — posting routines, job-queue handlers, integration flows — and make the test exercise that commit path. Pair the test codeunit with a `TestIsolation`-enabled test runner so committed changes are reverted at a higher scope. Pick `None` only for read-only tests or tests that drive UI code without writing from the test method itself. +Leave `[TransactionModel(...)]` undeclared to get the `AutoCommit` default when the codeunit's own tests rely on that default's behavior — for example a lazily-initialized shared fixture that commits once and cleans up its own scratch changes with a manual `asserterror`-based rollback (see `commit-shared-test-fixture-inside-lazy-initialize.md`); do not treat that absence as equivalent to declaring `AutoRollback`. When declaring `[TransactionModel(...)]` explicitly instead, pick `AutoRollback` for a test whose own logic and the code it exercises make no `Commit` call, `AutoCommit` when the code under test genuinely calls `Commit` — posting routines, job-queue handlers, integration flows — and make the test exercise that commit path, and `None` for a read-only test or one that drives UI code without writing from the test method itself. Pair an intentional, suite-wide reliance on `AutoCommit` with a `TestIsolation`-enabled test runner so committed changes are reverted at a higher scope. See sample: [`transactionmodel-attribute-governs-test-transactions.good.al`](transactionmodel-attribute-governs-test-transactions.good.al). ## Anti Pattern -Applying `AutoRollback` to every test method without checking whether the tested business logic calls `Commit`. The test throws at the first Commit, leaving no verdict on the behavior it intended to verify; in a CI run this looks like a flake or a setup bug, not a specification mismatch. The mirror-image anti-pattern is defaulting to `AutoCommit` across the suite "to avoid the error" — without a `TestIsolation` runner this permanently dirties the test database between runs and produces order-dependent test outcomes. +Declaring `[TransactionModel(AutoRollback)]` explicitly on a test method without checking whether the tested business logic calls `Commit`. The test throws at the first Commit, leaving no verdict on the behavior it intended to verify; in a CI run this looks like a flake or a setup bug, not a specification mismatch. The mirror-image anti-pattern is defaulting to `AutoCommit` across the suite "to avoid the error" — without a `TestIsolation` runner this permanently dirties the test database between runs and produces order-dependent test outcomes. Flagging a `Commit()` call in a test method that declares no `TransactionModel` attribute at all is not this anti-pattern — that shape does not error, and is BCApps' own documented pattern for shared lazy fixtures. See sample: [`transactionmodel-attribute-governs-test-transactions.bad.al`](transactionmodel-attribute-governs-test-transactions.bad.al). + +## Source + +The `AutoCommit`-is-default claim and the exact rollback-to-last-`Commit` mechanics are quoted from Microsoft's TransactionModel Property reference: https://learn.microsoft.com/en-us/previous-versions/dynamicsnav-2018-developer/TransactionModel-Property. The current AL [TransactionModel attribute](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/attributes/devenv-transactionmodel-attribute) page describes the same three values but never states a default; this older property reference is the citable source for that fact. diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al new file mode 100644 index 0000000..2acba18 --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al @@ -0,0 +1,27 @@ +codeunit 50104 "Item Price Testing" +{ + Subtype = Test; + + [Test] + procedure ApplyDiscount_LogicTest() + var + Assert: Codeunit "Library Assert"; + begin + // logic test — fine on its own, but not paired with a UI test below + Assert.AreEqual(90, ApplyDiscount(100, 10), 'A 10% discount on 100 must yield 90'); + end; + + local procedure ApplyDiscount(UnitPrice: Decimal; DiscountPct: Decimal): Decimal + begin + exit(UnitPrice - (UnitPrice * DiscountPct / 100)); + end; + + [Test] + procedure CustomerCard_Opens_UT() + var + CustomerCard: TestPage "Customer Card"; + begin + // UI test mixed into a logic-test codeunit, and the codeunit lacks the _UT suffix + CustomerCard.OpenNew(); + end; +} diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al b/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al new file mode 100644 index 0000000..6c343d8 --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al @@ -0,0 +1,42 @@ +codeunit 50105 "Item Price Testing" +{ + Subtype = Test; + + [Test] + procedure ApplyDiscount_ReducesUnitPrice() + var + Assert: Codeunit "Library Assert"; + DiscountedPrice: Decimal; + begin + DiscountedPrice := ApplyDiscount(100, 10); + Assert.AreEqual(90, DiscountedPrice, 'A 10% discount on 100 must yield 90'); + end; + + local procedure ApplyDiscount(UnitPrice: Decimal; DiscountPct: Decimal): Decimal + begin + exit(UnitPrice - (UnitPrice * DiscountPct / 100)); + end; +} + +codeunit 50106 "Item Price Testing_UT" +{ + Subtype = Test; + + [Test] + procedure CustomerCard_SetName_UpdatesField() + var + Customer: Record Customer; + CustomerCard: TestPage "Customer Card"; + Assert: Codeunit "Library Assert"; + LibrarySales: Codeunit "Library - Sales"; + begin + LibrarySales.CreateCustomer(Customer); + CustomerCard.OpenEdit(); + CustomerCard.GoToRecord(Customer); + CustomerCard.Name.SetValue('Updated Name'); + CustomerCard.Close(); + + Customer.Get(Customer."No."); + Assert.AreEqual('Updated Name', Customer.Name, 'Name must be updated through the page'); + end; +} diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.md b/microsoft/knowledge/testing/ui-test-codeunit-naming.md new file mode 100644 index 0000000..0790f37 --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [ui-test, testpage, naming, suffix, codeunit, page-testing, team-convention] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Separate UI-layer and logic-layer tests into different codeunits + +## Description + +A test codeunit that drives pages through `TestPage` — opening pages, reading FactBox parts, triggering field `OnValidate` through the page — is testing a different layer than a codeunit that calls business-logic procedures directly. Readers need to know which layer a given test exercises without opening it, and a single codeunit that mixes both kinds of test hides that distinction: a failure could mean the logic broke, the page broke, or both. The `_UT` suffix and adjacent-object-ID pairing below are one team's naming convention for making that split visible, not a BCApps-wide naming standard — BCApps itself uses `UT` for unit tests generally, not specifically to mean "UI layer," and does not treat adjacent object IDs as a semantic pairing mechanism. Apply the suffix only on a project that has explicitly adopted this convention. + +## Best Practice + +Keep UI-layer (`TestPage`-driven) and logic-layer tests in separate codeunits regardless of naming. Projects that adopt a `_UT`-style suffix convention should apply it consistently to every UI-layer test codeunit, keep the corresponding logic-only codeunit unsuffixed, and document the convention where the team's other naming rules live. + +See sample: [`ui-test-codeunit-naming.good.al`](ui-test-codeunit-naming.good.al). + +## Anti Pattern + +One codeunit that mixes a direct logic-call test and a `TestPage`-driven test side by side — a failing test no longer tells a reader which layer actually broke. On a project that has adopted the `_UT` convention, a UI-layer codeunit missing the suffix is also an instance of this anti-pattern; on a project that has not adopted it, the suffix itself is not required. + +See sample: [`ui-test-codeunit-naming.bad.al`](ui-test-codeunit-naming.bad.al). diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al new file mode 100644 index 0000000..07ad6b2 --- /dev/null +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al @@ -0,0 +1,18 @@ +codeunit 50143 "Sample Doc Amount Test" +{ + Subtype = Test; + + [Test] + procedure DocAmountIsNotVerifiedWhenLinesAreMissing() + var + Assert: Codeunit Assert; + PurchHeader: Record "Purchase Header"; + begin + asserterror Assert.IsTrue(VerifyDocAmount(PurchHeader), 'Doc. amount should not verify with no lines.'); + end; + + local procedure VerifyDocAmount(var PurchHeader: Record "Purchase Header"): Boolean + begin + exit(false); + end; +} diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.good.al b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.good.al new file mode 100644 index 0000000..eb21d6c --- /dev/null +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.good.al @@ -0,0 +1,18 @@ +codeunit 50143 "Sample Doc Amount Test" +{ + Subtype = Test; + + [Test] + procedure DocAmountIsNotVerifiedWhenLinesAreMissing() + var + Assert: Codeunit Assert; + PurchHeader: Record "Purchase Header"; + begin + Assert.IsFalse(VerifyDocAmount(PurchHeader), 'Doc. amount should not verify with no lines.'); + end; + + local procedure VerifyDocAmount(var PurchHeader: Record "Purchase Header"): Boolean + begin + exit(false); + end; +} diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md new file mode 100644 index 0000000..182a62b --- /dev/null +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md @@ -0,0 +1,34 @@ +--- +bc-version: [all] +domain: testing +keywords: [assert, isfalse, istrue, asserterror, boolean-check, negative-test] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use Assert.IsFalse to check a boolean result, not asserterror around Assert.IsTrue + +## Description + +`asserterror` exists to assert that a statement raises a runtime error; it is not a general-purpose way to invert a boolean check. Wrapping `asserterror Assert.IsTrue(SomeFunc(), Msg)` to verify that `SomeFunc()` returns `false` tests whether `Assert.IsTrue`'s own error-raising behavior fired, not the value `SomeFunc()` actually returned. + +## Best Practice + +When the code under test returns a `Boolean` rather than raising an error, assert the value directly with `Assert.IsFalse(SomeFunc(), Msg)` (or `Assert.IsTrue` for the positive case). Reserve `asserterror` for statements expected to actually raise an error. + +See sample: [`use-assert-isfalse-not-asserterror-for-boolean-checks.good.al`](use-assert-isfalse-not-asserterror-for-boolean-checks.good.al). + +## Anti Pattern + +`asserterror Assert.IsTrue(SomeFunc(), Msg);` to verify `SomeFunc()` is `false`. It passes today because `Assert.IsTrue` happens to raise an error on failure, but it verifies the assertion helper's error-raising behavior, not the value under test. + +See sample: [`use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al`](use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al). + +## Source + +Drawn from Luc van Vugt's "TDD in NAV – ASSERTERROR or IsFalse": https://www.fluxxus.nl/index.php/bc/tdd-in-nav-asserterror-or-isfalse/. The post's own example and reasoning — reserve `asserterror` for the product code actually raising an error, use `Assert.IsFalse`/`Assert.IsTrue` to check a boolean the test framework itself computes — carries over directly; the overlap with `asserterror-needs-expectederror-and-code.md` below is this repository's own addition, not from the source. + +## Scope + +This rule and `asserterror-needs-expectederror-and-code.md` can both match `asserterror Assert.IsTrue(SomeFunc(), Msg);` with nothing after it — the generic rule sees a bare `asserterror`, this one sees `asserterror` wrapping an `Assert.IsTrue`/`Assert.IsFalse` call used to invert a boolean. This rule wins for that shape: the fix is to replace the construct with a direct `Assert.IsFalse`/`Assert.IsTrue` call, not to add `Assert.ExpectedError`/`Assert.ExpectedErrorCode` after it. `asserterror-needs-expectederror-and-code.md` still applies on its own to every other bare `asserterror`, including one guarding `Assert.IsTrue`/`Assert.IsFalse` where the intent genuinely is to assert that the guarded call itself raises an error (for example, asserting that a validation helper errors before it can even return a boolean). diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al new file mode 100644 index 0000000..fd371f5 --- /dev/null +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al @@ -0,0 +1,21 @@ +page 50131 "Sample Item List" +{ + PageType = List; + SourceTable = "Sample Item"; + // Anti-pattern: no CardPageID even though a Card page exists for + // this table, and no UsageCategory, so the page is invisible to + // Tell Me search. + ApplicationArea = All; + + layout + { + area(content) + { + repeater(Group) + { + field(Description; Rec.Description) { } + field("No."; Rec."No.") { } // primary key buried, not left-most + } + } + } +} diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al new file mode 100644 index 0000000..7b53ac0 --- /dev/null +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al @@ -0,0 +1,20 @@ +page 50130 "Sample Item List" +{ + PageType = List; + SourceTable = "Sample Item"; + CardPageID = "Sample Item Card"; // links back to its Card page + UsageCategory = Lists; + ApplicationArea = All; + + layout + { + area(content) + { + repeater(Group) + { + field("No."; Rec."No.") { } // primary key, left-most + field(Description; Rec.Description) { } + } + } + } +} diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md new file mode 100644 index 0000000..18f7178 --- /dev/null +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md @@ -0,0 +1,90 @@ +--- +bc-version: [all] +domain: ui +keywords: [pages, page-design, naming-conventions, page-type, card-page, list-page, factbox, worksheet-page, document-page, rolecenter, cardpageid, autosplitkey] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Pages must match one of Business Central's page-type conventions + +## Description + +Business Central's page types — RoleCenter, Card, List, CardPart, +ListPart, Worksheet, Document, ListPlus, plus system dialog/special +types such as `NavigatePage`, `ConfirmationDialog`, `StandardDialog`, +`HeadlinePart`, and `API` (a selected list of conventional types this +article covers design conventions for — not an exhaustive catalogue of +every current `PageType` value; `PromptDialog`, `ConfigurationDialog`, +`UserControlHost`, and `XmlPort` also exist but follow their own +design rules, out of scope here) — each +fix a naming pattern and a structural constraint, not just a visual +layout. A page whose name, primary-key handling, or linkage +(`CardPageID`, `SubPageLink`, `AutoSplitKey`) doesn't match its own type's +conventions is either the wrong page type for the job or built +inconsistently with the rest of the application, and should be flagged in +review even if it compiles and renders. Before naming a new page or +wiring its links, first ask which page type it is, and whether the source +table actually fits that type's structural requirement — the type fixes +the naming suffix, which fields are visible, and which other page it must +link back to. + +## Best Practice + +Match the page's design to its type: + +- **RoleCenter** — tailored home page for a role; named role + `Role + Center`; links to List pages, shows Cues/Activities. +- **Card** — view/edit one record; named table + `Card`; FastTabs only, + first FastTab named `General`. A single-field primary key is typical, + but not a hard requirement: a subsidiary table that supplements a + master record with its own identity (parent key + own code — Ship-to + Address, Customer/Vendor Bank Account) commonly gets its own Card page + over a composite key too. Treat the key shape as a contextual signal, + not a mandatory constraint — a composite-key table with no such + supplementing relationship to a master record is the actual signal a + List/Worksheet/Tabular page fits better. +- **List** — view multiple records, also the lookup/drilldown surface; + named table + `List` if read-only, or the plural table name if + editable; primary-key fields shown left-most; `CardPageID` must point + at the associated Card page when one exists. +- **CardPart** — single-column FactBox; named for its content + + `FactBox`. +- **ListPart** — multi-column FactBox or subpage (e.g. document lines); + named for its content + `FactBox`/`SubPage`; `SubPageLink` must + actually filter to the host record. +- **Worksheet** — multi-record entry for a Journal-like table, insertion + order preserved; primary-key fields never shown; uses `AutoSplitKey` + with a trailing `Integer` key field. +- **Document** — FastTabs plus a lines subpage, lines filtered to the + header; named for the document (`Sales Invoice`). +- **ListPlus** — like Document but with multiple lists instead of one; + named like the record/report it summarizes. +- System dialog types (`NavigatePage`, `ConfirmationDialog`, + `StandardDialog`, `HeadlinePart`) are fixed shapes with no page-name + suffix convention. `API` pages follow their own property rules and are + extended by adding a new API page, never a page extension. + +Before wiring controls, the design step should fix: which users and +tasks the page serves, the concrete fields/commands/links those tasks +need, the page type that matches the content (chosen before the source +table), and the source table that actually holds the page's primary data. + +See sample: [`page-design-must-match-bc-page-type-conventions.good.al`](page-design-must-match-bc-page-type-conventions.good.al). + +## Anti Pattern + +A page that mixes conventions from two types — for example, a "List" +page with no `CardPageID` even though a Card page exists for the same +table — signals a design step was skipped, not a stylistic choice. A +Card page over a composite-key table is not automatically this anti +pattern; check whether the table supplements a master record first. Also watch +for a Worksheet or List page showing primary-key fields it shouldn't (or +hiding them when it should show them). A page with no `UsageCategory` set +is not automatically a defect either: supporting pages, subpages, dialogs, +and pages intended only to be reached through another workflow correctly +have no `UsageCategory` — flag its absence only on a page intended as a +searchable entry point in its own right. + +See sample: [`page-design-must-match-bc-page-type-conventions.bad.al`](page-design-must-match-bc-page-type-conventions.bad.al). diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al new file mode 100644 index 0000000..82065f0 --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al @@ -0,0 +1,32 @@ +local procedure UpgradeCustomerFields() +begin + if not UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then begin + Customer.SetLoadFields("Discount %", "Customer Posting Group"); + if Customer.FindSet() then + repeat + if (Customer."Discount %" = 0) and (Customer."Customer Posting Group" <> '') then begin + Customer."Discount %" := 5; + Customer.Modify(); + end; + until Customer.Next() = 0; + UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag()); + + // BUG: a second, unrelated migration's tag check nested inside the + // first migration's guarded body. Neither tag can be checked, + // skipped, or fixed independently of the other - a failure or a + // deliberate skip of the discount migration silently takes the + // shipping-agent migration down with it, and nothing in the + // Upgrade Tags table records that the second step ran on its own. + if not UpgradeTag.HasUpgradeTag(GetCustomerShippingAgentFieldTag()) then begin + Customer.SetLoadFields("Shipping Agent Code"); + if Customer.FindSet() then + repeat + if Customer."Shipping Agent Code" = '' then begin + Customer."Shipping Agent Code" := DefaultShippingAgentCode(); + Customer.Modify(); + end; + until Customer.Next() = 0; + UpgradeTag.SetUpgradeTag(GetCustomerShippingAgentFieldTag()); + end; + end; +end; diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al new file mode 100644 index 0000000..e121570 --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al @@ -0,0 +1,40 @@ +local procedure UpgradeCustomerDiscountField() +begin + if UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then + exit; + + Customer.SetLoadFields("Discount %", "Customer Posting Group"); + if Customer.FindSet() then + repeat + // A business-data safety condition inside this one migration's + // loop is not a second migration hiding inside the first - + // Microsoft's own upgrade-tag example nests exactly this shape + // (a corruption guard, then a redundant-write guard) inside a + // single tagged procedure. + if (Customer."Discount %" = 0) and (Customer."Customer Posting Group" <> '') then begin + Customer."Discount %" := 5; + Customer.Modify(); + end; + until Customer.Next() = 0; + + UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag()); +end; + +// A second, genuinely unrelated migration gets its own tag and its own +// top-level procedure - not nested inside the first one's guarded body. +local procedure UpgradeCustomerShippingAgentField() +begin + if UpgradeTag.HasUpgradeTag(GetCustomerShippingAgentFieldTag()) then + exit; + + Customer.SetLoadFields("Shipping Agent Code"); + if Customer.FindSet() then + repeat + if Customer."Shipping Agent Code" = '' then begin + Customer."Shipping Agent Code" := DefaultShippingAgentCode(); + Customer.Modify(); + end; + until Customer.Next() = 0; + + UpgradeTag.SetUpgradeTag(GetCustomerShippingAgentFieldTag()); +end; diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md new file mode 100644 index 0000000..d2384f8 --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md @@ -0,0 +1,34 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [upgrade-tag, nesting, complexity, upgrade-per-company, upgrade-per-database] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Never nest upgrade tag checks or blend two migrations under one tag + +## Description + +Upgrade tag *checks* should stay flat: never nest one tag's existence check inside another tag's guarded body, and never let one tagged procedure quietly perform a second, functionally distinct migration — that turns two upgrade steps into one that can't be tracked, skipped, or fixed independently, which is exactly what separate tags exist to prevent. That is the specific nesting Microsoft's own guidance warns against ("Keep tags simple by limiting nesting tags to two levels"). + +That is not a limit on how much conditional logic a single migration's own loop body may contain. Microsoft's own worked example for upgrade tags nests a record loop with two business-data safety conditions — a corruption guard, then a redundant-write guard — inside one `if UpgradeTagMgt.HasUpgradeTag(...) then exit;`-guarded procedure, and its own design guidance separately *requires* this: "Implement extra safety checks to avoid data corruption, even though you're using upgrade tags." A business-data guard that protects the single migration a tag represents is not a second migration hiding inside the first, however many `if` levels it takes. + +Upgrade code runs unattended, once, against production data with no chance to interactively debug a wrong branch — which is why mixing two migrations under one tag, or losing track of which tag guards which step, is a genuinely higher-cost mistake here than the equivalent would be in ordinary application code. + +## Best Practice + +One tag, one migration: exit early if the tag is already set, then run the one upgrade step that tag represents — including as many business-data safety conditions as that single step's own correctness requires, nested however deep the logic actually needs. Reach for a second, separately tagged migration only when the nested logic is doing genuinely unrelated work (a different table, a different field, a different concern) that could legitimately be skipped, retried, or fixed on its own. + +See sample: [`upgrade-tag-logic-must-not-nest-deeply.good.al`](upgrade-tag-logic-must-not-nest-deeply.good.al). + +## Anti Pattern + +Checking one upgrade tag inside the guarded body of another, or writing two functionally unrelated migrations — different tables, different concerns — under a single tag so neither can be tracked, skipped, or fixed independently of the other. A record loop with business-data safety conditions inside one tagged migration's own body is not this anti-pattern, even several `if` levels deep, as long as every condition serves that one migration. + +See sample: [`upgrade-tag-logic-must-not-nest-deeply.bad.al`](upgrade-tag-logic-must-not-nest-deeply.bad.al). + +## Source + +Microsoft's own "Upgrading Extensions" guidance, Design considerations: "Keep tags simple by limiting nesting tags to two levels. Complicated if statements can lead to problems." — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-upgrading-extensions#using-upgrade-tags-to-control-upgrade-code diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al new file mode 100644 index 0000000..13c33b4 --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al @@ -0,0 +1,25 @@ +page 50100 "Vendor Document API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'documents'; + APIVersion = 'v1.0'; + EntityName = 'vendorDocument'; + EntitySetName = 'vendorDocuments'; + SourceTable = Vendor; + // no InsertAllowed/ModifyAllowed override, no Editable = false anywhere + + layout + { + area(content) + { + repeater(GroupName) + { + field(no; Rec."No.") { } + field(vatRegNo; Rec."VAT Registration No.") { } + field(contactEmail; Rec."E-Mail") { } + // ...dozens more fields, none marked Editable = false + } + } + } +} diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al new file mode 100644 index 0000000..b524b4f --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al @@ -0,0 +1,25 @@ +page 50102 "Vendor Contact Info API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'integration'; + APIVersion = 'v1.0'; + EntityName = 'vendorContact'; + EntitySetName = 'vendorContacts'; + SourceTable = Vendor; + DelayedInsert = true; + InsertAllowed = false; + DeleteAllowed = false; + + layout + { + area(content) + { + repeater(GroupName) + { + field(no; Rec."No.") { Editable = false; } + field(contactEmail; Rec."E-Mail") { } + } + } + } +} diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md new file mode 100644 index 0000000..6f40c87 --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: web-services +keywords: [api-page, least-privilege, write-access, odata, security, external-api, identity-fields] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Give API pages least-privilege write access + +## Description + +A general-purpose API page that exposes many fields should not be widened to allow writes on one additional field. A `PageType = API` page consumed by an external integration, an automation agent, or a partner system carries the same risk regardless of caller: a write-enabled page with no per-field restriction is a wide-open surface. Least privilege has to cover both dimensions of exposure: which fields are on the page, and which operations the page allows. Only a field actually placed on the page is reachable at all — but a page that includes many fields, with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults and no `Editable = false` on most of them, leaves every one of those included fields — identity fields and financially significant ones among them — fully writable, with nothing marking that as deliberate. Restricting fields alone is not enough either: a page with only two fields on it can still let a caller insert brand-new records or delete existing ones if `InsertAllowed`/`DeleteAllowed` are left at their true defaults (both `true`). + +## Best Practice + +Create a separate, minimal API page that exposes only the key and the specific field the consumer needs to write, with everything else `Editable = false` or simply absent from the page — and set `InsertAllowed`/`DeleteAllowed` to `false` unless the consumer's use case genuinely needs to create or delete records through that page. + +See sample: [`api-page-least-privilege-write-access.good.al`](api-page-least-privilege-write-access.good.al). + +## Anti Pattern + +Widening an existing general-purpose API page with write access to one field, leaving every other field on the page (including identity and posting fields) writable by default because no one added `Editable = false`. + +See sample: [`api-page-least-privilege-write-access.bad.al`](api-page-least-privilege-write-access.bad.al). diff --git a/microsoft/skills/review/al-appsource-review.md b/microsoft/skills/review/al-appsource-review.md index 2efc049..c12815f 100644 --- a/microsoft/skills/review/al-appsource-review.md +++ b/microsoft/skills/review/al-appsource-review.md @@ -52,6 +52,7 @@ The following targeted checks cover every current `appsource` article across the - A page or report that repository context identifies as a direct user entry point omits `UsageCategory` or sets it to `None` — `set-usagecategory-on-searchable-entry-points`. Do not select this article based only on object type; exclude supporting parts, dialogs, API pages, and objects intentionally reached through another page. - A `DateTime` assignment adds or subtracts a fixed duration to represent an assumed regional offset — `do-not-hard-code-time-zone-offsets`. Require contextual evidence such as an hour-sized constant, offset-oriented name, or time-zone comment; do not flag deadlines, schedules, or elapsed-time calculations. - For BC v27 or later, `app.json` adds or changes the `help` URL to a path deeper than two levels, or a changed Copilot/context-sensitive help arrangement would ground the app under an overly broad truncated parent — `keep-copilot-help-url-to-two-path-levels`. +- A release/submission pipeline change (`AL-Go-Settings.json`, a publish/release workflow) or an `app.json` version bump is present without the new complete version being strictly greater than the previously submitted one, or the change asserts a hand-edited build/revision or every-merge-is-a-release policy as a universal AppSource rule rather than a project-specific workflow choice — `release-must-update-app-version`. Require repository/pipeline context to know the previously submitted version; a single `app.json` diff cannot prove ordering on its own. - Changed code declares or calls `File.Open`/`File.Create`/`File.Read`/`File.Write` in an app targeting Business Central Online — `file-datatype-saas`. 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-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md index 2f747fc..5f4cec7 100644 --- a/microsoft/skills/review/al-data-modeling-review.md +++ b/microsoft/skills/review/al-data-modeling-review.md @@ -46,6 +46,9 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `data-modeling` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action. - A `* Setup` table or its page changes singleton structure, uses a nonblank or generated key, permits insert/delete, uses a List page, or does not ensure the blank-keyed row exists — `setup-table-is-a-singleton`. +- A new field is typed `Media`, `MediaSet`, or `BLOB` and the field's caption/name suggests a picture or image — `pictures-must-use-media-not-blob`. +- Code outside a test codeunit or a demo-data generator calls `WorkDate(NewDate)` (the assignment form, not a bare `WorkDate()` read) as part of logic whose purpose is unrelated to the work date itself — `code-must-not-change-workdate`. A test deliberately setting a date context, or a demo-data routine that saves, sets, and restores the work date to backdate the data it creates, is not this anti-pattern. +- A new or extended table's name, fields, or usage positively establish it as one of Business Central's nine business-record types — a name ending `Ledger Entry`/`Register`/`Journal Line`/`Header`/`Line`/`Setup`, an auto-generated `Entry No.`/`No.` key posted from elsewhere, a `Template Name`+`Batch Name`+`Line No.` key, or a singleton `Primary Key` field — `table-design-must-match-bc-table-type-conventions`. Do not worklist it from a bare `keys` block or primary-key declaration alone: a temporary/buffer table, a work queue, a log, a cross-reference/mapping table, or a process-local staging table is not one of the nine types and is out of this rule's scope entirely, not an unresolved case. - Code reads `Item Ledger Entry."Document No."` (or `"Last Shipping No."`/`"Last Posting No."`) after a combined Ship+Invoice **sales** post — `item-ledger-entry-document-no-follows-last-shipping-no`. This is a sales-specific rule: purchase combined posting is Receive+Invoice and uses receiving fields such as `"Last Receiving No."`, not the shipment/document-number behavior this article describes. Do not worklist it from purchase posting code. - A custom master table changes its primary key, `No.`/`No. Series` fields, or `OnInsert` without assigning a blank `No.` from setup through a number series — `master-table-no-from-number-series-in-oninsert`. - BC v22 or later code introduces or retains `NoSeriesManagement`, `InitSeries`, `SelectSeries`, or `SetSeries`, or number assignment/manual-entry checks do not use codeunit `"No. Series"` methods such as `GetNextNo`, `IsManual`, or `TestManual` — `use-no-series-codeunit-not-noseriesmanagement`. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index a340807..3962363 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -55,6 +55,8 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `error-handling` article: - `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`. +- New or changed code inserts an error/duration log record around a failed `TryFunction`/`GetLastErrorText`/`GetLastErrorCode` path and then raises, propagates, or rethrows the error — `log-writes-must-survive-rollback`. Do not worklist it when the log insert already happens inside a `Session.StartSession`-targeted codeunit's `OnRun`; that is the compliant shape, not the signal to flag. +- A guarded lookup (`if Record.Get(...) then ... else` or similar) sets a value used later, and the same guard shape (with the same blank/zero fallback style) is applied to a field that feeds a posted amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output — `defensive-vs-offensive-code-must-match-blast-radius`. The signal is a posting-critical or compliance-facing field guarded defensively with a silent fallback, not the mere presence of a guarded lookup. - Developer-only invariant text is raised with default client visibility, or a user-actionable validation is hidden as `ErrorType::Internal` — `errortype-internal-vs-client-for-diagnostics`. - `FieldError` receives a complete capitalized sentence, repeats the field caption/value, or ends the predicate with punctuation — `fielderror-default-message-logic`. - An unguarded `FieldError` is used as though it performed a comparison, or `TestField` is forced onto a complex rule needing a tailored predicate — `fielderror-vs-testfield`. diff --git a/microsoft/skills/review/al-performance-review.md b/microsoft/skills/review/al-performance-review.md index 6109f7b..6eae659 100644 --- a/microsoft/skills/review/al-performance-review.md +++ b/microsoft/skills/review/al-performance-review.md @@ -39,13 +39,16 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially tables, pages with SourceTable bindings, reports, queries, and codeunits performing record iteration. - The changed procedures and triggers, weighted toward those that perform loops, Find/FindSet/FindFirst calls, CalcFields, SetAutoCalcFields, CalcSums, FlowField access, Commit calls, checkpoint helpers, record copying, RecordRef conversion, Modify/Delete calls, or cross-table navigation. -- Tokens extracted from the diff that relate to data access, hot-path costs, and background scheduling (`SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `ReadIsolation`, `LockTable`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`, `FlowField`, `Visible`, `Job Queue Entry`, `Job Queue Category Code`, `Confirm`, `RunModal`, `GuiAllowed`, `TryFunction`, `Codeunit.Run`, `HttpClient`, `Status`, `On Hold`, `stop request`, `TaskScheduler.CreateTask`, `TaskScheduler.TaskExists`). +- Tokens extracted from the diff that relate to data access, hot-path costs, and background scheduling (`key`, `IncludedFields`, `SumIndexFields`, `SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `FindLast`, `IsEmpty`, `ReadIsolation`, `LockTable`, `Insert`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Validate`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`, `CalcFormula`, `FlowField`, `Query.Open`, `Query.Read`, `Visible`, `Job Queue Entry`, `Job Queue Category Code`, `Confirm`, `RunModal`, `GuiAllowed`, `TryFunction`, `Codeunit.Run`, `HttpClient`, `Status`, `On Hold`, `stop request`, `TaskScheduler.CreateTask`, `TaskScheduler.TaskExists`, `Page.RunModal`, `Report.RunModal`, `Report.Run`, `Xmlport.Run`, `UseRequestPage`). 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. Apply these targeted cues even when simple token overlap would rank the article below the worklist cutoff: -- Worklist `use-setautocalcfields-for-per-row-flowfields.md` when a record loop calls `CalcFields`, or when every row reads the same FlowField for a comparison, branch, or per-record action. Worklist `calcsums-instead-of-calcfields-in-loop.md` instead when the loop only accumulates one set total. +- Worklist `design-covering-keys-from-read-pattern.md` for a changed secondary key alongside a filtered reader of its table, and `review-overlapping-keys-before-adding-an-index.md` when added or changed keys have overlapping leading fields. A changed key alone is not a finding; read and write workloads determine whether either rule applies. +- Worklist `preserve-buffered-inserts-by-separating-target-reads.md` when a loop calls `Insert` and interleaves operations on the insert target or `Commit`. Worklist `aggregate-before-persisting-intermediate-results.md` when repeated grouping calculations and persistent intermediate summaries appear in the same processing path. Do not infer either pattern from `Insert` or `CalcSums` alone. +- Worklist `cache-repeated-filtered-results-with-explicit-scope.md` only when the code or workload establishes repeated **complete** lookup keys (for example, querying the same filtered set in multiple passes, or measured key reuse), or shows a cache that omits result-affecting inputs. A single loop over possibly distinct keys does not establish reuse or justify a cache finding. Worklist `avoid-repeating-unchanged-validation.md` for repeated `Validate` of the same field in one path; do not worklist it from a single validation call. +- Worklist `use-setautocalcfields-for-per-row-flowfields.md` when a record loop calls `CalcFields`, or when every row reads the same FlowField for a comparison, branch, or per-record action. Also worklist `calcsums-instead-of-calcfields-in-loop.md` when the loop accumulates one set total: use `CalcSums` directly only for stored source fields, never directly on FlowFields; a FlowField total requires deriving equivalent source filters from its `CalcFormula`. - Worklist `hidden-flowfields-still-calculate-before-bc26-opt-in.md` when a page control directly sources a FlowField and sets `Visible = false` or a visibility expression. Suppress it when the target is known to have BC26's **Calculate only visible FlowFields** feature enabled, or when the FlowField is cheap and intentionally preloaded. - Worklist `avoid-commit-inside-loops.md` when `Commit()` is inside a record-iteration body or a checkpoint loop lacks persisted progress that excludes completed work on retry. Do not match a commit after a complete business unit when the same transaction persists a restart-safe watermark/state and errors propagate. Still match a full-tail `FindSet` with periodic commits as unbounded retrieval; restart safety does not make it `TOP X`. - Worklist `prefer-modifyall-over-per-row-modify.md` for a constant-assignment `Modify(false)` loop with no validation or per-row semantics. Worklist `triggers-and-media-field-regress-modifyall.md` when table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields affect a bulk path. A progress dialog does not generically exempt a loop; accept it only when the equivalent bulk call already falls back to individual operations and semantics are preserved. @@ -58,6 +61,7 @@ Apply these targeted cues even when simple token overlap would rank the article - Worklist `job-queue-on-hold-does-not-stop-running-work.md` when a running job queue handler polls the entry's `Status` or `On Hold` value as a cancellation signal. Exclude application-owned stop requests that are checked before every bounded unit of work, including the first, when completed work and its checkpoint remain consistent and resume logic clears the request. - Worklist `job-queue-category-code-serializes-conflicting-jobs.md` when two or more job queue entries in the same company are shown by the changed context to require mutual exclusion but have empty or different Job Queue Category Codes. Do not infer a conflict merely because jobs touch the same tables, and do not recommend a category to coordinate across companies, environments, or workers outside the job queue dispatcher. - Worklist `store-scheduled-task-id-to-avoid-duplicate-tasks.md` when `TaskScheduler.CreateTask` runs from initialization, login, setup, or another repeatable path without persisting its returned GUID and checking it with `TaskScheduler.TaskExists` before creating a replacement. Exclude one-shot creation and correctly persisted check-before-create flows; concurrent callers still require serialization around that sequence. +- Worklist `al-methods-limited-during-write-transactions.md` when `Page.RunModal` follows an `Insert`, `Modify`, or `Delete` in the same trigger or procedure with no intervening `Commit`; or when `Report.RunModal`/`Report.Run` without `false` as its `RequestWindow` argument and without `UseRequestPage(false)` follows one; or when `Xmlport.Run` without `false` as its `RequestWindow` argument and without the `UseRequestPage = false` object property follows one. Do not worklist it from a call that precedes every write, from a report run with its request page suppressed via `UseRequestPage(false)`/`Run(...,false)`/`RunModal(...,false)`, or from an XMLport run with its request page suppressed via the `RequestWindow` argument or the `UseRequestPage` property. A `Codeunit.Run` whose return value is used in that position belongs to `codeunit-run-requires-prior-commit-inside-transaction.md`. - A new or changed report object whose usage/name/caption identifies it as a single-record document (invoice, statement, order confirmation) sets or retains `DefaultRenderingLayout = RDLC` — `document-report-word-layout.md`. Do not worklist this from a tabular/list report with heavy aggregation or calculated columns; RDLC/Excel remains the better fit there. These targeted inclusions and exclusions override generic token overlap. Do not retain an excluded article solely because the diff contains one of its keywords. diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md index e18d3fc..3d70173 100644 --- a/microsoft/skills/review/al-security-review.md +++ b/microsoft/skills/review/al-security-review.md @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially permission sets, codeunits handling authentication or authorization, objects touching `Isolated Storage`, `OAuth2` flows, web service endpoints, API pages, event publishers, and RecordRef helpers. - The changed procedures and triggers, weighted toward those that call `HttpClient`, validate or compose URLs, write to telemetry, read or write secrets, unwrap SecretText, manipulate record-level security, expose var Boolean guard parameters, or bypass the permission model (for example, `RecordRef.Open`, `Record.WritePermission`, direct table access from a non-owning app). -- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`, `SetFilter`). +- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`, `SetFilter`, `ServiceEnabled`, `PageType = API`, `QueryType = API`, `permissionset`, `Web Services`). 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. @@ -49,6 +49,7 @@ For secret values, select the most specific sink owner: - When a `Text`/`Code` credential is declared, passed, returned, or unwrapped without a visible HTTP URI/header/body sink, use `secrettext-for-credentials.md`. - When that value is interpolated into a URI, authorization header, or HTTP body and sent through `HttpClient`, use `secrettext-with-httpclient.md` as the primary finding. It supersedes the generic credential-type article at that location; keep the latter only as a supporting reference when useful. +- When a page/query is registered in Web Services, declares `PageType = API`/`QueryType = API`, a codeunit is registered in Web Services, or `[ServiceEnabled]` is added to a page procedure, and no permission set in the app grants a matching `page "..." = X` / `query "..." = X` / `codeunit "..." = X` entry for that specific object — use `exposed-objects-must-be-in-a-permission-set.md`. The anti-pattern is the exposed object missing its own execute entry, even when the underlying table's `tabledata` permissions look complete; require repository-level permission-set context, since one file cannot prove an entry is absent elsewhere in the app. 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-style-review.md b/microsoft/skills/review/al-style-review.md index 0cc747e..2aaf902 100644 --- a/microsoft/skills/review/al-style-review.md +++ b/microsoft/skills/review/al-style-review.md @@ -52,6 +52,9 @@ Apply these high-signal mappings before fuzzy topic ranking: - A `Label` or `TextConst` contains multiple or ambiguous placeholders but has no `Comment`, or its Comment does not explain every placeholder — `label-comment-explains-placeholders.md`. A single placeholder whose meaning is explicit in the text, such as `Customer %1`, is allowed without a Comment and must not be flagged. - A normal two-argument `Evaluate` has a resolved `DateFormula` destination and a hard-coded non-angle-bracket date-formula literal, directly or through a visible constant — `dateformula-evaluate-needs-language-independent-literals.md`. Do not use this cue for dynamic/localized external input, already invariant `<...>` input, or direct `CalcDate(Text, ...)` calls. - `function-call-parentheses-required.md` applies only to a zero-argument invocation written without `()`. Never worklist it from an invocation that already has parentheses or supplies arguments, including `Error(Label, Arg1, Arg2)`. +- A new or changed comment restates what the adjacent code already makes obvious from its own names and structure (a comment that just repeats a variable/field/method name in prose) rather than explaining a non-obvious constraint, invariant, or workaround — `al-comments-must-not-restate-what-code-already-shows.md`. A comment absent entirely is not this anti-pattern; only a present-but-redundant comment is. +- A `page`/`pageextension` adds or changes a procedure body that performs a calculation, validation, or record mutation belonging to a business operation reused across entry points, rather than presentation-specific state or a call into a codeunit — `pages-must-not-contain-business-logic.md`. A page calling a codeunit procedure, or a page's own presentation-only state and formatting, is not this anti-pattern; nor is a data invariant that belongs on the table itself. +- Changed source files are added under an object-type folder (`Tables/`, `Pages/`, `Codeunits/`, etc.) in a repository whose existing structure is predominantly feature-based, or vice versa — `source-organized-by-feature-not-object-type.md`. The anti-pattern is inconsistency with the repository's own established convention, not the choice of either scheme; a repository consistently organized by object type throughout is not a violation. Require repository-level folder context; a single new file's path cannot prove the project's convention alone. - A new `.app` build artifact appears at the project root or another unversioned/arbitrary location, or is added to source control alongside the AL source that produced it — `al-build-output-must-not-pollute-project-root.md`. A deliberate `--outfolder`/`outputPath` destination added to `.gitignore` is the compliant shape, not the signal to flag. - A new or renamed AL identifier (variable, procedure, parameter, field, object, enum value, or label identifier) contains non-English words — `al-identifiers-english.md`. A caption, tooltip, or other user-facing text value in a non-English language is not this anti-pattern; only the identifier itself is in scope. - A field or variable is typed `Boolean` and its two possible values are genuinely named domain alternatives (a status pair like Inbound/Outbound, Debit/Credit, Buy/Sell) rather than a true/false predicate, or an `Option`/`Enum`/`Integer` models a domain concept that is intrinsically a yes/no flag — `binary-choice-must-be-boolean.md`. The signal is a semantic mismatch between the type and the domain concept, not the current number of states. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 2adbc13..c42fe85 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -46,11 +46,19 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `testing` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action. - A method in a `Subtype = Test` codeunit adds or changes `[TransactionModel(...)]`, exercises code that calls `Commit` under `AutoRollback`, defaults broadly to `AutoCommit`, or chooses `None` for a writing test — `transactionmodel-attribute-governs-test-transactions`. +- A new or changed `[Test]` procedure is added, whether or not it already carries `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` tags — `test-feature-scenario-tags`. A procedure with no tags at all, or a generic name like `Test1`, is the anti-pattern signal; presence of the tags is the compliant shape, not the thing to search for. +- A test codeunit calls `TestPage` methods (`OpenNew`, `OpenView`, `OpenEdit`) alongside `[Test]` procedures in the same codeunit that call business-logic procedures directly with no `TestPage` involved — `ui-test-codeunit-naming`. The anti-pattern signal is both kinds of test mixed into one codeunit (or, on a project using the `_UT` convention, a UI-layer codeunit missing the suffix); a codeunit containing only `TestPage`-driven tests is not itself a violation. +- A `[GIVEN]`-tagged setup precedes a posting call or report execution and does not visibly set up posting-group/VAT setup records, an explicit date, or (for a report test) both an included and an excluded record — `given-blocks-must-cover-full-precondition-chain`. +- A test procedure contains more than one `[WHEN]` block, or more than one distinct action not labelled `[GIVEN]`, without the procedure name declaring a flow/defect-then-fix shape — `test-one-when-per-test`. +- A `BCPT*` scenario codeunit is added and the PerformanceTest app's only other scenario codeunits are copies of Microsoft's shipped BCPT samples (`BCPT Create Customer`, `BCPT Create Item Journal`, `BCPT Post GL Entries`, etc.) with no scenario exercising the extension's own codeunits, FlowFields, or pages — `bcpt-scenarios-must-be-app-specific`. - An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it. - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`. - Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`. - A test codeunit's `Initialize` procedure exits on `IsInitialized` before per-test reset such as `LibraryVariableStorage.Clear`, `LibrarySetupStorage.Restore`, or `LibraryTestInitialize.OnTestInitialize`, or a `[Test]` method in a codeunit using that pattern does not call `Initialize()` first — `reset-per-test-state-before-the-isinitialized-guard`. -- `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`. +- `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`. Exclude `asserterror Assert.IsTrue(...)` / `asserterror Assert.IsFalse(...)` only when it is used solely to invert the guarded call's Boolean result (the same condition `use-assert-isfalse-not-asserterror-for-boolean-checks` cues on below, which wins for that shape) — not when the test expects the guarded Boolean-returning call itself to raise an error, which this rule still owns even though it happens to wrap an `Assert.IsTrue`/`IsFalse` call. Also exclude a trailing `asserterror Error(...)` used purely as an end-of-test rollback sentinel after a lazy `Initialize()` fixture already committed — that shape belongs to `commit-shared-test-fixture-inside-lazy-initialize`, which wins for it; the sentinel's own error text is not meant to be asserted against. +- `asserterror` wraps `Assert.IsTrue(BooleanExpression, ...)` (or the `IsFalse` mirror) solely to invert the boolean result of the guarded call, rather than to assert that call itself raises an error — `use-assert-isfalse-not-asserterror-for-boolean-checks`. +- A shared/lazy `Initialize()`-style fixture helper creates fixture data without a following `Commit()`, in a test method whose body later forces its own rollback (for example `asserterror Error(...)` used for end-of-test cleanup) — `commit-shared-test-fixture-inside-lazy-initialize`. The presence of `Commit()` after the fixture is the compliant shape, not the signal to look for; the missing-`Commit()` shape combined with a later deliberate rollback is the anti-pattern. Require runner/repository context for the `TestIsolation` value: a standalone test file cannot prove which runner executes it, and under `Function`-level isolation this whole pattern is moot regardless of `Commit()` — do not raise the finding when the executing runner's `TestIsolation` is known to be `Function`. +- Changed code subscribes to `OnAfterRemoveTableRelation`, calls `RemoveTableRelation`, or references `Codeunit "Table Relation Test"`/134926 — `table-relation-test-exclude-known-invalid-relations-via-event`. - Test fixture code assigns a hardcoded literal to a primary-key field or a field the test relies on as a unique lookup identifier, hand-builds a "unique" value for such a field (string concatenation, a counter, `Format(CurrentDateTime)`), or truncates `LibraryUtility.GenerateGUID()`'s result with `CopyStr` for such a field shorter than 10 characters — `use-generateguid-for-unique-test-fixture-values`. Calling `GenerateGUID()` untruncated into a full-length field, `GenerateRandomCodeWithLength` for a shorter field needing real verified uniqueness, or `GenerateRandomCode20` specifically for a `Code[20]` field, is the compliant shape, not the signal to flag. `GenerateRandomCode20` is not a substitute for `GenerateRandomCodeWithLength` on a shorter field — it truncates `GenerateGUID()`'s sequential value down to the field's length by keeping the *leftmost* characters, which change the slowest, so retries against a short field can churn through the same truncated prefix far longer than `GenerateRandomCodeWithLength`'s equivalent. A hardcoded or deterministic value in an ordinary descriptive field is not this anti-pattern — that field carries no uniqueness constraint. Do not claim `GenerateRandomCode` (without `WithLength`/`20`) or `GenerateRandomXMLText` verify uniqueness against the real table, or that `GenerateRandomCode` is collision-free even within one test run for a short field — none of that is true. - A test asserts against a `TestPage` field's `.Visible()` or `.Enabled()` — `use-testpage-visible-enabled-to-verify-field-ui-state`. When the assertion is against `.Editable()`, or the page is opened with `OpenEdit()` specifically to check editability — `use-testpage-editable-to-verify-field-editability`. - A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification. diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index 8c35f49..f54c0c9 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -40,8 +40,9 @@ Discard files that are not applicable. Retain conditionally applicable files onl Narrow the relevant files to the subset that applies to the changes under review. - **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files. +- A new or changed page's name/suffix, primary-key handling, `CardPageID`, `SubPageLink`, `AutoSplitKey`, or `UsageCategory` doesn't match the conventions of its own declared `PageType` — `page-design-must-match-bc-page-type-conventions.md`. A Card page over a composite-key table that supplements a master record, or a supporting/subpage/dialog page intended only to be reached through another workflow and correctly omitting `UsageCategory`, is not this anti-pattern on its own; check whether the page is actually mixing conventions or is meant as a searchable entry point before flagging. - For each relevant knowledge file, compute overlap against changed page declarations and control add-in files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, promoted action definitions, field importance, page background tasks, DOM creation, ARIA attributes, keyboard/focus handlers, packaged-resource AJAX, and calls from JavaScript into AL. -- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions). +- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`). 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 page element. 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-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 9ae61f8..dce1053 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -45,6 +45,7 @@ Narrow the relevant files to the subset that applies to the changes under review - 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 `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. 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. diff --git a/microsoft/skills/review/al-web-services-review.md b/microsoft/skills/review/al-web-services-review.md index b86c79b..6d7d71a 100644 --- a/microsoft/skills/review/al-web-services-review.md +++ b/microsoft/skills/review/al-web-services-review.md @@ -42,6 +42,7 @@ Narrow the relevant files to the subset that applies to the changes under review - Outbound HTTP integration code that constructs or sends requests, captures a client method's optional Boolean result, checks an `HttpResponseMessage`, or reads and parses response content. - Integration code that converts values to or from exchanged text: `Format` or `Evaluate` on a request URL, body, or file line, and `JsonToken`/`JsonValue` conversions of payload properties. - Webhook subscriber handlers and subscription lifecycle code, especially code that creates or renews subscriptions, handles `validationToken`, schedules from `expirationDateTime`, or targets resources whose eligibility is visible in the diff. +- An API page (`PageType = API`) is added or changed and its `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` properties are left at their default `true` (or explicitly set `true`) for an operation the endpoint's stated purpose does not need — `api-page-least-privilege-write-access.md`. The signal is an operation left enabled beyond what the endpoint's own described purpose requires, not the mere presence of these properties; a page that genuinely needs full CRUD and grants it deliberately is not a violation. - An API page (`PageType = API`) with `InsertAllowed = true` lists a field in `ODataKeyFields` and that same field control sets `Editable = false` — `api-page-key-fields-must-be-editable-on-insert.md`. A system-generated key such as `SystemId` marked read-only is not this anti-pattern. - An API page exposes a field via `Rec` directly, and that field is a stored value computed from other fields inside an `OnValidate` trigger rather than a FlowField recalculated in `OnAfterGetRecord` — `stored-derived-fields-must-not-be-exposed-directly.md`. Exposing a genuine FlowField, or a value already recalculated in `OnAfterGetRecord`, is not this anti-pattern. - Tokens extracted from the diff that relate to API surface and behaviour (`PageType`, `QueryType`, `API`, `api-page`, `page-part`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SystemId`, `SubPageLink`, `subpagelink`, `Multiplicity`, `multiplicity`, `Many`, `ZeroOrOne`, `SourceTableTemporary`, `Job Queue Entry`, `webhook`, `webhookSupportedResources`, `webhook-supported-resources`, `subscriptions`, `notificationUrl`, `validationToken`, `validationtoken`, `expirationDateTime`, `expirationdatetime`, `ServiceEnabled`, `WebServiceActionContext`, `SetActionResponse`, `ReadIsolation`, `IsolationLevel`, `ReadCommitted`, `InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`, `SourceTable`, `HttpClient`, `HttpRequestMessage`, `HttpResponseMessage`, `Get`, `Post`, `Put`, `Delete`, `Send`, `IsSuccessStatusCode`, `HttpStatusCode`, `Content`, `ReadAs`, `JsonObject`, `JsonToken`, `JsonValue`, `AsValue`, `IsNull`, `SelectToken`, `Format`, `Evaluate`, `XmlDocument`).