From a4d85c3e9edcff8e7156b966d38797d92237de76 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:59:54 +0200 Subject: [PATCH] Add 18 more community AL/BC patterns across appsource, data-modeling, error-handling, security, style, testing, ui, upgrade, and web-services Second contribution from CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Cross-checked against the current microsoft/knowledge corpus before opening; several originally-drafted candidates were dropped as duplicates of existing files. --- .../release-must-update-app-version.md | 37 +++++++++ .../code-must-not-change-workdate.bad.al | 9 +++ .../code-must-not-change-workdate.good.al | 11 +++ .../code-must-not-change-workdate.md | 40 +++++++++ .../pictures-must-use-media-not-blob.bad.al | 11 +++ .../pictures-must-use-media-not-blob.good.al | 11 +++ .../pictures-must-use-media-not-blob.md | 44 ++++++++++ ...ust-match-bc-table-type-conventions.bad.al | 17 ++++ ...st-match-bc-table-type-conventions.good.al | 16 ++++ ...gn-must-match-bc-table-type-conventions.md | 81 +++++++++++++++++++ ...ensive-code-must-match-blast-radius.bad.al | 11 +++ ...nsive-code-must-match-blast-radius.good.al | 10 +++ ...-offensive-code-must-match-blast-radius.md | 26 ++++++ .../log-writes-must-survive-rollback.bad.al | 24 ++++++ .../log-writes-must-survive-rollback.good.al | 26 ++++++ .../log-writes-must-survive-rollback.md | 26 ++++++ ...objects-must-be-in-a-permission-set.bad.al | 12 +++ ...bjects-must-be-in-a-permission-set.good.al | 10 +++ ...sed-objects-must-be-in-a-permission-set.md | 26 ++++++ ...not-restate-what-code-already-shows.bad.al | 18 +++++ ...ot-restate-what-code-already-shows.good.al | 20 +++++ ...ust-not-restate-what-code-already-shows.md | 30 +++++++ ...ges-must-not-contain-business-logic.bad.al | 20 +++++ ...es-must-not-contain-business-logic.good.al | 31 +++++++ .../pages-must-not-contain-business-logic.md | 31 +++++++ ...ce-organized-by-feature-not-object-type.md | 36 +++++++++ ...bcpt-scenarios-must-be-app-specific.bad.al | 29 +++++++ ...cpt-scenarios-must-be-app-specific.good.al | 43 ++++++++++ .../bcpt-scenarios-must-be-app-specific.md | 26 ++++++ ...-must-cover-full-precondition-chain.bad.al | 12 +++ ...must-cover-full-precondition-chain.good.al | 15 ++++ ...ocks-must-cover-full-precondition-chain.md | 26 ++++++ .../testing/test-feature-scenario-tags.bad.al | 21 +++++ .../test-feature-scenario-tags.good.al | 26 ++++++ .../testing/test-feature-scenario-tags.md | 26 ++++++ .../testing/test-one-when-per-test.bad.al | 15 ++++ .../testing/test-one-when-per-test.good.al | 29 +++++++ .../testing/test-one-when-per-test.md | 34 ++++++++ .../testing/ui-test-codeunit-naming.bad.al | 24 ++++++ .../testing/ui-test-codeunit-naming.good.al | 33 ++++++++ .../testing/ui-test-codeunit-naming.md | 26 ++++++ ...must-match-bc-page-type-conventions.bad.al | 21 +++++ ...ust-match-bc-page-type-conventions.good.al | 20 +++++ ...ign-must-match-bc-page-type-conventions.md | 76 +++++++++++++++++ ...rade-tag-logic-must-not-nest-deeply.bad.al | 14 ++++ ...ade-tag-logic-must-not-nest-deeply.good.al | 14 ++++ .../upgrade-tag-logic-must-not-nest-deeply.md | 28 +++++++ ...i-page-least-privilege-write-access.bad.al | 23 ++++++ ...-page-least-privilege-write-access.good.al | 21 +++++ .../api-page-least-privilege-write-access.md | 26 ++++++ 50 files changed, 1262 insertions(+) create mode 100644 microsoft/knowledge/appsource/release-must-update-app-version.md create mode 100644 microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al create mode 100644 microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al create mode 100644 microsoft/knowledge/data-modeling/code-must-not-change-workdate.md create mode 100644 microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al create mode 100644 microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al create mode 100644 microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md create mode 100644 microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al create mode 100644 microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al create mode 100644 microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md create mode 100644 microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al create mode 100644 microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al create mode 100644 microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md create mode 100644 microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al create mode 100644 microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al create mode 100644 microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md create mode 100644 microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al create mode 100644 microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al create mode 100644 microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md create mode 100644 microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al create mode 100644 microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al create mode 100644 microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md create mode 100644 microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al create mode 100644 microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al create mode 100644 microsoft/knowledge/style/pages-must-not-contain-business-logic.md create mode 100644 microsoft/knowledge/style/source-organized-by-feature-not-object-type.md create mode 100644 microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al create mode 100644 microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al create mode 100644 microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md create mode 100644 microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al create mode 100644 microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al create mode 100644 microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md create mode 100644 microsoft/knowledge/testing/test-feature-scenario-tags.bad.al create mode 100644 microsoft/knowledge/testing/test-feature-scenario-tags.good.al create mode 100644 microsoft/knowledge/testing/test-feature-scenario-tags.md create mode 100644 microsoft/knowledge/testing/test-one-when-per-test.bad.al create mode 100644 microsoft/knowledge/testing/test-one-when-per-test.good.al create mode 100644 microsoft/knowledge/testing/test-one-when-per-test.md create mode 100644 microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al create mode 100644 microsoft/knowledge/testing/ui-test-codeunit-naming.good.al create mode 100644 microsoft/knowledge/testing/ui-test-codeunit-naming.md create mode 100644 microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al create mode 100644 microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al create mode 100644 microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md create mode 100644 microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al create mode 100644 microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al create mode 100644 microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md create mode 100644 microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al create mode 100644 microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al create mode 100644 microsoft/knowledge/web-services/api-page-least-privilege-write-access.md 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..91fb093 --- /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 version number is the only identity a deployed app has. Two customer environments running "the same" version with different code is an undiagnosable support case; an AppSource submission with an unchanged major.minor is a rejected submission. AL-Go increments build numbers on every CI run, which creates the illusion that versioning is handled — but major.minor is a human statement about compatibility, and no pipeline can make it. + +## Best Practice + + Before the release merge: + app.json: "version": "1.3.0.0" (new functionality -> minor bump) + AL-Go settings: "repoVersion": "1.3" (where used) + Then: feature branch -> main via PR, tag, release. + +Feature branches never touch the version; only the release does. + +## 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..428e6db --- /dev/null +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md @@ -0,0 +1,40 @@ +--- +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). Application code must never call the `WorkDate` +function to set a new value. Doing so changes what the user sees and +defaults to for the rest of their session, as a side effect of running +unrelated business logic — a surprising, hard-to-trace behavior change the +user never asked for and has no visibility into. + +This is a call-direction distinction: reading the current work date via +`WorkDate` (or `WorkDate()` with no argument) is fine and common — it is +only the assignment form, `WorkDate(NewDate)`, that is the anti-pattern. + +## Best Practice + +Read the work date to default a value; never write to it. + +See sample: `code-must-not-change-workdate.good.al`. + +## Anti Pattern + +Setting the work date from within a codeunit, report, or page action +changes session state the user owns, for the duration of a call that has +nothing to do with the user's date preference. If a scenario genuinely +needs a specific date for a calculation, pass or compute that date as a +local variable — never repurpose the session's `WorkDate`. + +See sample: `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..3786d47 --- /dev/null +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md @@ -0,0 +1,44 @@ +--- +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 image variants (e.g. multiple +sizes). Media/MediaSet integrate with the platform's picture control, the +media repository, image caching, and thumbnail generation — none of which +a plain `BLOB` field gets. A `BLOB` field storing a picture works, but it +is the legacy approach: no caching, no thumbnail support, and no +integration with the standard picture controls used across Business +Central pages. + +`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` (or `MediaSet` for multiple image variants) for any field that +holds a picture. + +See sample: `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, caching, and thumbnail +generation 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. + +See sample: `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..01f2793 --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md @@ -0,0 +1,81 @@ +--- +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.`; + Worksheet or Tabular page, always filtered by the calling page. +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. + +See sample: `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`. 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..fb822b3 --- /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 SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then + VATRegNo := SalesHeader."VAT Registration No."; // 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..38fbc88 --- /dev/null +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al @@ -0,0 +1,10 @@ +// Low blast radius: guard, with an explicit chosen fallback. +if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then + VATRegNo := SalesHeader."VAT Registration No."; +// Blank is an acceptable, deliberately-considered default here - the field +// is informational and a reviewer sees it before the document ships. + +// 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..fab6850 --- /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`. + +## 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 VAT registration number shown only 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. + +See sample: `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..34b22ec --- /dev/null +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al @@ -0,0 +1,26 @@ +codeunit 50100 "Sample Error Log Writer" +{ + // Started via Session.StartSession so its commit is independent of the + // caller's transaction. Does one thing: insert the log entry, commit. + trigger OnRun() + var + ErrorLogEntry: Record "Sample Error Log"; + begin + ErrorLogEntry.Init(); + ErrorLogEntry."Call Duration (ms)" := CallDurationMs; + ErrorLogEntry."Error Message" := + CopyStr(ErrorMessageText, 1, MaxStrLen(ErrorLogEntry."Error Message")); + ErrorLogEntry.Insert(true); + Commit(); + end; + + procedure SetParameters(Duration: Integer; ErrorText: Text) + begin + CallDurationMs := Duration; + ErrorMessageText := ErrorText; + end; + + var + CallDurationMs: Integer; + ErrorMessageText: Text; +} 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..6939d01 --- /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 (`StartSession` on a codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. Capture duration and other telemetry values in the caller and pass them as parameters — the isolated session must not re-read state that a rollback may have erased. 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`. + +## 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`. 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..75f1378 --- /dev/null +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md @@ -0,0 +1,26 @@ +--- +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 — an API page (`PageType = API`), a web-service-enabled page or query (`ServiceEnabled = true`), or a published API query — is only usable if it is also granted execute access through a permission set. 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 (`page "..." = X`, `query "..." = X`) in a permission set shipped by the app. 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` / `ServiceEnabled`) rather than leaving an orphaned endpoint with no permission-set membership. + +See sample: `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`. 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..2ff120a --- /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] +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`. + +## 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`. 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..333625d --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al @@ -0,0 +1,20 @@ +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + actions + { + area(Processing) + { + action(Recalculate) + { + trigger OnAction() + begin + Rec."Total 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..a7a40b1 --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al @@ -0,0 +1,31 @@ +codeunit 50100 "Sales Line Management" +{ + procedure RecalculateLine(var SalesLine: Record "Sales Line") + begin + SalesLine."Total Amount" := SalesLine.Quantity * SalesLine."Unit Price"; + SalesLine.Modify(); + end; +} + +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + actions + { + area(Processing) + { + action(Recalculate) + { + trigger OnAction() + begin + SalesLineMgt.RecalculateLine(Rec); + end; + } + } + } + + var + SalesLineMgt: Codeunit "Sales 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..d113a79 --- /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] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep business logic out of page objects + +## Description + +A page procedure that calculates a value and assigns it to a field, calls `Rec.Modify()` directly, or implements a business rule is an architecture violation even when it compiles. Pages are a presentation layer: they bind data to the UI and invoke actions. Calculations, validations, and record mutations belong in codeunits, where they can be tested, reused, and called consistently regardless of which page (or API, or batch job) triggers them. When logic lives on a page, it only applies when a user opens that specific page — the same business rule silently doesn't run through any other entry point. + +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`. + +## Anti Pattern + +Direct calculations in a page trigger (e.g. `Rec."Total Amount" := Rec.Quantity * Rec."Unit Price"`), calls to `Rec.Modify()` from a page trigger, or business-rule validation embedded in `OnValidate`/`OnAction` instead of routed through a codeunit. + +See sample: `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..eb1e574 --- /dev/null +++ b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md @@ -0,0 +1,36 @@ +--- +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 + +Source folders inside an AL app should group files by the business feature or module they belong to (`src/Sales/Invoice/`, `src/NoSeries/`), not by which kind of AL object they are (`src/Tables/`, `src/Pages/`, `src/Codeunits/`). Object-type folders scatter everything belonging to one feature across half a dozen directories, so a developer picking up a feature has to jump between folders that share nothing but object type to see the whole picture. Feature folders keep a table, its pages, its codeunits, and its test setup physically together. + +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 + + src/ + ├── Tables/ + ├── Pages/ + └── Codeunits/ + +Finding everything related to one feature now requires searching multiple folders and mentally reassembling it from scattered pieces. 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..0ca29de --- /dev/null +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al @@ -0,0 +1,43 @@ +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"; + IsInitialized: Boolean; + + local procedure InitTest() + begin + // Set up any required configuration + end; + + local procedure CreateServiceRequest(var BCPTTestContext: Codeunit "BCPT Test Context") + begin + BCPTTestContext.StartScenario('Create Service Request Header'); + // ... create the service request + BCPTTestContext.EndScenario('Create Service Request Header'); + BCPTTestContext.UserWait(); + + BCPTTestContext.StartScenario('Add Service Request Line'); + // ... add a line + 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..8dae0b7 --- /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: `SingleInstance = true`, implementing `"BCPT Test Param. Provider"`, wrapping the operation under test in `BCPTTestContext.StartScenario()` / `EndScenario()`, and building its own test data in a local `InitTest()` procedure rather than depending on hardcoded records. Give each distinct step its own named scenario so a regression in one step doesn't hide inside a coarser measurement. + +See sample: `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`. 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..4357b40 --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al @@ -0,0 +1,12 @@ +[Test] +procedure PostSalesOrder_CreatesInvoice() +var + SalesHeader: Record "Sales Header"; +begin + // [GIVEN] a sales order — posting groups left to whatever exists in the test company + LibrarySales.CreateSalesOrder(SalesHeader); + // [WHEN] + LibrarySales.PostSalesOrder(SalesHeader, false, true); + // [THEN] + 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..1a509fc --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al @@ -0,0 +1,15 @@ +[Test] +procedure PostSalesOrder_CreatesInvoice() +var + Customer: Record Customer; + SalesHeader: Record "Sales Header"; +begin + // [GIVEN] a customer with a full posting-group chain and VAT setup + LibrarySales.CreateCustomerWithPostingSetup(Customer); + // [GIVEN] a sales order for that customer, dated explicitly + LibrarySales.CreateSalesOrderForCustomer(SalesHeader, Customer."No.", WorkDate()); + // [WHEN] + LibrarySales.PostSalesOrder(SalesHeader, false, true); + // [THEN] + 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..71e63e5 --- /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`. + +## 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`. 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..fa9d2c0 --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al @@ -0,0 +1,21 @@ +codeunit 50102 "Item Price Testing" +{ + Subtype = Test; + + var + ItemPriceMgt: Codeunit "Item Price Mgt."; + Assert: Codeunit "Library Assert"; + + [Test] + procedure Test1() + var + Customer: Record Customer; + Item: Record Item; + Price, Disc: Decimal; + begin + // setup mixed with assertions, no clear layers + Customer.Insert(false); + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', Price, Disc); + Assert.AreEqual(100, 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..6866403 --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al @@ -0,0 +1,26 @@ +// [FEATURE] Item Price — price cascade (Customer -> Price Group -> All Customers) +codeunit 50103 "Item Price Testing" +{ + Subtype = Test; + + var + LibrarySales: Codeunit "Library - Sales"; + ItemPriceMgt: Codeunit "Item Price Mgt."; + Assert: Codeunit "Library Assert"; + + // [SCENARIO] Customer with a specific price list line gets that unit price + [Test] + procedure GetPrice_CustomerPrice_ReturnsUnitPrice() + var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; + begin + // [GIVEN] a customer with a price list line at 100 LCY + LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100); + // [WHEN] + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + // [THEN] + Assert.AreEqual(100, UnitPrice, '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..5842bee --- /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] +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. Put `[SCENARIO]` immediately above each `[Test]` attribute, describing the scenario in business language that complements — not duplicates — the procedure name. Inside the body, mark the precondition setup as `[GIVEN]`, the single action under test as `[WHEN]`, and the assertions as `[THEN]`. 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`. + +## 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`. 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..d24da5d --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.bad.al @@ -0,0 +1,15 @@ +[Test] +procedure GetPrice_ThenGetDiscount_ReturnsCorrectValues() +var + TempBuffer: Record "Item Price Tier Buffer" temporary; + UnitPrice, LineDiscPct: Decimal; +begin + // [GIVEN] ... + // [WHEN] first action + ItemPriceMgt.GetSalesPrice(CustomerNo, ItemNo, '', UnitPrice, LineDiscPct); + // [WHEN] second action — this is a second test in disguise + ItemPriceMgt.GetSalesPriceTiers(CustomerNo, ItemNo, '', TempBuffer); + // [THEN] asserting two unrelated things + Assert.AreEqual(100, UnitPrice, ''); + Assert.IsFalse(TempBuffer.IsEmpty(), ''); +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..320bffd --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.good.al @@ -0,0 +1,29 @@ +[Test] +procedure GetPrice_CustomerPrice_ReturnsCorrectUnitPrice() +var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; +begin + // [GIVEN] a customer with a price list line at 100 + LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100); + // [WHEN] + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + // [THEN] + Assert.AreEqual(100, UnitPrice, 'Unit price must match price list'); +end; + +[Test] +procedure GetPriceTiers_CustomerTier_ReturnsOneTierLine() +var + Customer: Record Customer; + Item: Record Item; + TempBuffer: Record "Item Price Tier Buffer" temporary; +begin + // [GIVEN] a customer with a tier price at min qty 10 + LibrarySales.CreateCustomerWithTierPrice(Customer, Item, '', 10, 90); + // [WHEN] + ItemPriceMgt.GetSalesPriceTiers(Customer."No.", Item."No.", '', TempBuffer); + // [THEN] + Assert.AreEqual(1, TempBuffer.Count(), 'Exactly one tier line 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..8aa6ca8 --- /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 + +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`. + +## 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`. + +## 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/ui-test-codeunit-naming.bad.al b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al new file mode 100644 index 0000000..98fc2b2 --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al @@ -0,0 +1,24 @@ +codeunit 50104 "Item Price Testing" +{ + Subtype = Test; + + [Test] + procedure GetPrice_LogicTest() + var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; + begin + // logic test — fine on its own, but not paired with a UI test below + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + end; + + [Test] + procedure Page_ShowsPrice_UT() + var + ItemPricePage: TestPage "Item Price"; + begin + // UI test mixed into a logic-test codeunit, and the codeunit lacks the _UT suffix + ItemPricePage.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..5f216b0 --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al @@ -0,0 +1,33 @@ +codeunit 50105 "Item Price Testing" +{ + Subtype = Test; + + [Test] + procedure GetPrice_CustomerPrice_ReturnsUnitPrice() + var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; + begin + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + end; +} + +codeunit 50106 "Item Price Testing_UT" +{ + Subtype = Test; + + [Test] + procedure Page_EnterCustomerAndItem_FactBoxShowsPrice() + var + Customer: Record Customer; + Item: Record Item; + ItemPricePage: TestPage "Item Price"; + Assert: Codeunit "Library Assert"; + begin + ItemPricePage.OpenNew(); + ItemPricePage.CustomerNo.SetValue(Customer."No."); + ItemPricePage.ItemNo.SetValue(Item."No."); + Assert.AreEqual('100.00', ItemPricePage.PriceInfo.UnitPrice.Value(), ''); + 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..4166cd7 --- /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] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Suffix UI-layer test codeunits with _UT and never mix layers in one codeunit + +## 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. + +## Best Practice + +Give any test codeunit that uses `TestPage` a `_UT` (Unit Test — UI layer) suffix, and keep it free of tests that call codeunit/table procedures directly. Keep the corresponding logic-only codeunit unsuffixed. Allocate the two codeunits adjacent object IDs so their relationship is visible in the object list. + +See sample: `ui-test-codeunit-naming.good.al`. + +## Anti Pattern + +A codeunit named without the `_UT` suffix that nonetheless contains `TestPage` calls, or — worse — one codeunit that mixes a direct logic-call test and a `TestPage`-driven test side by side. Either way, the codeunit's name no longer tells a reader which layer a failing test actually broke. + +See sample: `ui-test-codeunit-naming.bad.al`. 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..5f925d2 --- /dev/null +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md @@ -0,0 +1,76 @@ +--- +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 the system dialog types +(Navigate, ConfirmationDialog, StandardDialog, HeadlinePart, API) — 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`. Requires a single-field primary key — a + multi-field key needs a List/Worksheet/Tabular page instead. +- **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 (`Navigate`, `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`. + +## Anti Pattern + +A page that mixes conventions from two types — for example, a "Card" +page built on a table with a two-field primary key, or 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. Also watch +for: a Worksheet or List page showing primary-key fields it shouldn't (or +hiding them when it should show them), and a page with no +`UsageCategory` set, which makes it invisible to Tell Me search even +though it otherwise works. + +See sample: `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..c6eaeba --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al @@ -0,0 +1,14 @@ +local procedure UpgradeCustomerDiscountField() +begin + if not UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then begin + if Customer.FindSet() then begin + repeat + if Customer."Discount %" = 0 then begin + if Customer."Customer Posting Group" <> '' then + Customer."Discount %" := 5; + end; + until Customer.Next() = 0; + end; + UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag()); + 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..a6f09a4 --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al @@ -0,0 +1,14 @@ +local procedure UpgradeCustomerDiscountField() +begin + if UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then + exit; + + Customer.SetLoadFields("Discount %"); + if Customer.FindSet() then + repeat + Customer."Discount %" := 5; + Customer.Modify(); + until Customer.Next() = 0; + + UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag()); +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..3bd7e97 --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [upgrade-tag, nesting, complexity, upgrade-per-company, upgrade-per-database] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep upgrade tag conditional logic to at most two levels of nesting + +## Description + +The conditional logic that gates upgrade code behind an upgrade tag should be at most two levels deep: check the tag, exit if already applied, otherwise run the upgrade step. Deeper nesting — tag checks inside tag checks, or a tag check combined with multi-branch business-data conditions — signals that the upgrade step is trying to do more than one thing, or that it is reconstructing decision logic that belongs in the tag structure itself (one tag per distinct upgrade step), not in nested `if` statements inside a single step. + +Upgrade code runs unattended, once, against production data with no chance to interactively debug a wrong branch. The cost of a nesting-driven mistake here is much higher than in ordinary application code, which is why the ceiling is lower than general AL style would otherwise allow. + +## Best Practice + +One tag check, one exit, one upgrade action — two levels deep at most. + +See sample: `upgrade-tag-logic-must-not-nest-deeply.good.al`. + +## Anti Pattern + +Nesting the tag check, a record loop, and a multi-branch business condition inside one procedure. Split the buried business condition into its own, separately tagged upgrade step instead. + +See sample: `upgrade-tag-logic-must-not-nest-deeply.bad.al`. 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..272fdf6 --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al @@ -0,0 +1,23 @@ +page 50100 "Vendor Document API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'documents'; + APIVersion = 'v1.0'; + 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..35af72b --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al @@ -0,0 +1,21 @@ +page 50102 "Vendor Contact Info API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'integration'; + APIVersion = 'v1.0'; + SourceTable = Vendor; + DelayedInsert = true; + + 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..5289fad --- /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. The most common real-world shape of this problem is not a page that started narrow and got widened — it is a page that was never restricted at all: with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults and no `Editable = false` on any field, every field on the source table — including identity fields and financially significant ones — is fully writable, with nothing marking that as deliberate. + +## 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. + +See sample: `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`.