diff --git a/.github/custom-layer-autoclose.md b/.github/custom-layer-autoclose.md new file mode 100644 index 0000000..f0b059e --- /dev/null +++ b/.github/custom-layer-autoclose.md @@ -0,0 +1,31 @@ +Hey @{{AUTHOR}} πŸ‘‹ + +First off β€” thank you for jumping in and experimenting! It's awesome to see people pushing on the framework. πŸŽ‰ + +That said, let me gently redirect you, because I think there's a small but important misunderstanding about how the `custom` layer is meant to work: + +The `custom` layer in *this* repo isn't a destination for PRs β€” it's the designated sandbox inside **your own fork**. Think of it as the "your timeline" branch of the multiverse 🌌: this repo is canon, your fork is where you get to remix the lore without needing anyone's approval. That's the whole point of the layer existing β€” so you *don't* have to upstream your team-specific or experimental work. + +The intended workflow is: + +1. 🍴 **Fork** BCQuality to your own GitHub account +2. Clone *your fork* locally +3. Drop your custom agents and knowledge into the `custom` layer **there** +4. Commit and push to your fork β€” no PR back to upstream needed for custom stuff + +That way you get full control, your changes survive upstream updates cleanly, and you can pull in new core releases from this repo whenever you want. ✨ + +**Now β€” here's the fun part:** if while building out your fork you discover knowledge, patterns, or agents that you think would genuinely benefit *everyone* using BCQuality (not just your team), that's exactly what the `/community` layer is for! 🌟 PRs to `/community` here in the upstream repo are absolutely welcome and encouraged β€” it's how the collective hive mind 🧠 levels up. So please: tinker in your fork, and when you strike gold that's worth sharing, send it our way via `/community`. + +Going to close this PR for now (since it's targeting `custom` rather than `/community`), but please don't read it as a "no" β€” it's a "yes, but let's route it correctly." πŸ™ Happy to help if you hit any snags spinning up your fork, and genuinely looking forward to seeing what you contribute to `/community` down the line. + +
+Files in this PR that triggered the auto-close + +{{FILES}} +
+ +May your merges be conflict-free. πŸš€ + +--- +πŸ€– This PR was closed automatically by the `Guard custom layer` workflow because it adds or changes content under `/custom/`. If you were only updating the template (`custom/README.md` or a `.gitkeep`), a maintainer can re-open it. If you think this was closed in error, just comment here. diff --git a/.github/new-top-level-flag.md b/.github/new-top-level-flag.md new file mode 100644 index 0000000..3d297ea --- /dev/null +++ b/.github/new-top-level-flag.md @@ -0,0 +1,10 @@ + +πŸ‘‹ Heads up @{{AUTHOR}} β€” and cc maintainers β€” this PR introduces **new top-level entries** that aren't part of BCQuality's known repository structure: + +{{ENTRIES}} + +This isn't a block β€” just a flag. 🚩 New top-level folders and files are *usually* unintended (a stray export, a tool's scratch dir, or content that meant to land inside an existing layer like `/community/knowledge/`). BCQuality keeps a deliberately small root: `.github/`, `community/`, `custom/`, `microsoft/`, `skills/`, and `tools/`, plus a handful of root docs. + +**If this was intentional** and the new entry genuinely belongs at the repo root, a maintainer can review and merge as normal β€” no action needed beyond a quick sanity check. **If it wasn't**, please move the content into the right existing layer (or drop it) and push an update. πŸ™ + +A maintainer will take a look before merging. diff --git a/.github/scripts/validate_frontmatter.py b/.github/scripts/validate_frontmatter.py index d969cef..682a801 100644 --- a/.github/scripts/validate_frontmatter.py +++ b/.github/scripts/validate_frontmatter.py @@ -59,7 +59,7 @@ MAX_KNOWLEDGE_LINES = 100 KEBAB_CASE = re.compile(r"^[a-z0-9]+(-[a-z0-9]+)*$") ISO_ALPHA2 = re.compile(r"^[a-z]{2}$") -RANGE_SHORTHAND = re.compile(r"^(\d+)\.\.(\d+)$") +RANGE_SHORTHAND = re.compile(r"^(\d+)\.\.(\d+)?$") FENCED_CODE_BLOCK = re.compile(r"^```", re.MULTILINE) HEADING_H2 = re.compile(r"^##\s+(.+?)\s*$", re.MULTILINE) @@ -149,6 +149,8 @@ def expand_bc_version(value: Any) -> tuple[list[int] | str | None, str | None]: """Return (expanded, error-message). One of the two is None. For the universal sentinel ["all"], `expanded` is the string "all". + For an open-ended range like ["26.."], `expanded` is the normalized + string "26.." (it cannot be enumerated; consumers match target >= 26). Otherwise it is the expanded list of version integers. """ if not isinstance(value, list) or not value: @@ -163,15 +165,19 @@ def expand_bc_version(value: Any) -> tuple[list[int] | str | None, str | None]: if any(v <= 0 for v in value): return None, "integers must be positive" return sorted(set(value)), None - # Case 2: single-element range-shorthand like "[26..28]" + # Case 2: single-element range shorthand β€” closed "[26..28]" or open-ended "[26..]" if len(value) == 1 and isinstance(value[0], str): m = RANGE_SHORTHAND.match(value[0].strip()) if m: - start, end = int(m.group(1)), int(m.group(2)) + start = int(m.group(1)) + if m.group(2) is None: + # Open-ended: "start.." applies from start onwards, no upper bound. + return f"{start}..", None + end = int(m.group(2)) if start > end: return None, f"range '{value[0]}' is not ascending" return list(range(start, end + 1)), None - return None, "must be [all], a list of integers, or a single-element range shorthand like [26..28]" + return None, "must be [all], a list of integers, or a range shorthand like [26..28] or [26..]" def headings_in_order(body: str) -> list[tuple[str, int]]: @@ -498,9 +504,48 @@ class SkillRecord: skill_id: str | None +def validate_sub_skills_registry(path: Path, fm: dict[str, Any], root: Path, report: Report) -> None: + """R26: a super-skill's declared `sub-skills` must exactly match the + `al-*-review.md` leaf files present in the same directory (set equality, + ordering-agnostic). This keeps the registered leaf list the single source + of truth and fails CI on a forgotten, stale, or missing registration. + + Only applies to action-skill files declaring a non-empty list-of-str + `sub-skills`. Files whose `sub-skills` is malformed are handled by R20. + """ + ss = fm.get("sub-skills") + if not is_non_empty_list_of_str(ss): + return + + declared = {s.lstrip("./") for s in ss} + + # Sibling leaves on disk, excluding the super-skill file itself. + leaves = { + p.relative_to(root).as_posix() + for p in path.parent.glob("al-*-review.md") + if p.resolve() != path.resolve() + } + + # Declared entries that are not real sibling leaves on disk (missing/stale). + for entry in sorted(declared - leaves): + entry_path = root / entry + if not entry_path.exists(): + report.error(path, "R26", f"declared sub-skill does not exist on disk: {entry}", 1) + else: + report.error( + path, "R26", + f"sub-skills entry is not a sibling 'al-*-review.md' leaf: {entry}", 1, + ) + + # Sibling leaves on disk that were never registered ('forgot to wire it up'). + for leaf in sorted(leaves - declared): + report.error(path, "R26", f"leaf not registered in sub-skills: {leaf}", 1) + + def run(root: Path) -> Report: report = Report() skill_records: list[SkillRecord] = [] + action_skill_fms: list[tuple[Path, dict[str, Any]]] = [] # Walk declared top-level folders only; avoid wandering into .git, etc. walk_roots = [root / "skills"] + [root / layer for layer in LAYERS] @@ -527,6 +572,8 @@ def run(root: Path) -> Report: validate_knowledge(path, parsed, report) elif kind == "action-skill": validate_action_skill(path, parsed, report) + if parsed.frontmatter: + action_skill_fms.append((path, parsed.frontmatter)) if parsed.frontmatter and isinstance(parsed.frontmatter.get("id"), str): skill_records.append(SkillRecord(path, "action-skill", parsed.frontmatter["id"])) elif kind == "meta": @@ -560,6 +607,10 @@ def run(root: Path) -> Report: others = [q.relative_to(root).as_posix() for q in paths if q != p] report.error(p, "R24", f"skill id '{sid}' ({kind}) is not unique; also defined in: {others}") + # Fourth pass: R26 sub-skills registry matches leaf files on disk + for path, fm in action_skill_fms: + validate_sub_skills_registry(path, fm, root, report) + return report diff --git a/.github/workflows/flag-new-top-level.yml b/.github/workflows/flag-new-top-level.yml new file mode 100644 index 0000000..4d5f1d7 --- /dev/null +++ b/.github/workflows/flag-new-top-level.yml @@ -0,0 +1,108 @@ +name: Flag new top-level entries + +# BCQuality keeps a deliberately small repository root. New top-level folders +# or files are almost always unintended β€” a stray export, a tool's scratch +# directory, or content that meant to land inside an existing layer (e.g. +# /community/knowledge/). PR #55 leaked exactly this kind of stray folder. +# +# Unlike the custom-layer guard, this workflow does NOT close the PR. It only +# posts a single advisory comment so a maintainer (and the author) can eyeball +# the addition. It reads the PR's file LIST via the API and never checks out or +# runs PR code. + +on: + pull_request_target: + types: [opened, reopened, synchronize] + +permissions: + contents: read + pull-requests: write + issues: write + +jobs: + flag: + if: github.repository == 'microsoft/BCQuality' + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + with: + sparse-checkout: | + .github/new-top-level-flag.md + sparse-checkout-cone-mode: false + + - name: Flag unexpected new top-level entries + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + + // Known, intended repository root. Anything else added at the root + // is flagged for a human to eyeball. + const ALLOWED_DIRS = new Set([ + '.github', 'community', 'custom', 'microsoft', 'skills', 'tools', + ]); + const ALLOWED_FILES = new Set([ + '.gitignore', 'CODEOWNERS', 'LICENSE', 'README.md', + 'SECURITY.md', 'agent-consumption.md', + ]); + + const MARKER = ''; + const { owner, repo } = context.repo; + const prNumber = context.payload.pull_request.number; + + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number: prNumber, per_page: 100, + }); + + // Only consider newly-added paths β€” a new top-level entry can only + // appear via an added file. + const added = files + .filter((f) => f.status === 'added') + .map((f) => f.filename); + + const newDirs = new Set(); + const newFiles = new Set(); + for (const p of added) { + const slash = p.indexOf('/'); + if (slash === -1) { + // Top-level file. + if (!ALLOWED_FILES.has(p)) newFiles.add(p); + } else { + // Top-level directory. + const dir = p.slice(0, slash); + if (!ALLOWED_DIRS.has(dir)) newDirs.add(dir); + } + } + + if (newDirs.size === 0 && newFiles.size === 0) { + core.info('No unexpected new top-level entries. Nothing to flag.'); + return; + } + + // Idempotency: don't re-flag on every synchronize. + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: prNumber, per_page: 100, + }); + if (comments.some((c) => c.body && c.body.includes(MARKER))) { + core.info('Already flagged on this PR. Skipping duplicate comment.'); + return; + } + + const lines = []; + for (const d of [...newDirs].sort()) lines.push(`- πŸ“ \`${d}/\` (new top-level folder)`); + for (const f of [...newFiles].sort()) lines.push(`- πŸ“„ \`${f}\` (new top-level file)`); + const entries = lines.join('\n'); + + core.warning(`Unexpected new top-level entries: ${[...newDirs, ...newFiles].join(', ')}`); + + let body = fs.readFileSync('.github/new-top-level-flag.md', 'utf8'); + body = body + .replace(/{{AUTHOR}}/g, context.payload.pull_request.user.login) + .replace(/{{ENTRIES}}/g, entries); + + await github.rest.issues.createComment({ + owner, repo, issue_number: prNumber, body, + }); + + core.info(`Flagged PR #${prNumber}.`); diff --git a/.github/workflows/guard-custom-layer.yml b/.github/workflows/guard-custom-layer.yml new file mode 100644 index 0000000..c061389 --- /dev/null +++ b/.github/workflows/guard-custom-layer.yml @@ -0,0 +1,88 @@ +name: Guard custom layer + +# The /custom/ layer is a template: in upstream microsoft/BCQuality it stays +# empty by default (README.md + .gitkeep placeholders only). Custom knowledge +# and skills are partner/customer-specific and belong in a fork, never upstream. +# +# This workflow auto-closes any PR that adds or changes content under /custom/ +# (anything beyond the allowed template files). It runs only on the upstream +# repo, so forks that legitimately populate /custom/ are unaffected. +# +# pull_request_target is required so the workflow runs with a token that can +# comment on and close the PR (including PRs opened from forks). It only reads +# the PR's file LIST via the API and never checks out or executes PR code, so +# the elevated token is not exposed to untrusted content. + +on: + pull_request_target: + types: [opened, reopened, synchronize] + +permissions: + contents: read + pull-requests: write + issues: write + +jobs: + guard: + # Never run on forks β€” a fork's /custom/ content is exactly what's supposed + # to live there. + if: github.repository == 'microsoft/BCQuality' + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + with: + sparse-checkout: | + .github/custom-layer-autoclose.md + sparse-checkout-cone-mode: false + + - name: Close PR if it touches the custom layer + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + + // Files under custom/ that ARE allowed to change (the template seed). + const ALLOWED = new Set([ + 'custom/README.md', + ]); + // Any .gitkeep under custom/ is also allowed. + const isAllowed = (p) => + ALLOWED.has(p) || /^custom\/.*\.gitkeep$/.test(p) || p === 'custom/.gitkeep'; + + const { owner, repo } = context.repo; + const prNumber = context.payload.pull_request.number; + + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number: prNumber, per_page: 100, + }); + + // Offending = added/modified/renamed/copied/changed paths under custom/ + // that are not template files. (We ignore pure deletions.) + const offending = files + .filter((f) => f.status !== 'removed') + .map((f) => f.filename) + .filter((p) => p.startsWith('custom/') && !isAllowed(p)); + + if (offending.length === 0) { + core.info('No disallowed /custom/ changes found. Nothing to do.'); + return; + } + + core.warning(`PR #${prNumber} touches the custom layer: ${offending.join(', ')}`); + + const fileList = offending.map((p) => `- \`${p}\``).join('\n'); + let body = fs.readFileSync('.github/custom-layer-autoclose.md', 'utf8'); + body = body + .replace(/{{AUTHOR}}/g, context.payload.pull_request.user.login) + .replace(/{{FILES}}/g, fileList); + + await github.rest.issues.createComment({ + owner, repo, issue_number: prNumber, body, + }); + + await github.rest.pulls.update({ + owner, repo, pull_number: prNumber, state: 'closed', + }); + + core.info(`Closed PR #${prNumber}.`); diff --git a/CODEOWNERS b/CODEOWNERS index c4fb72e..084e70a 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1,9 +1,18 @@ # Microsoft-endorsed content and skills require maintainer review -/microsoft/ @jeschulz -/skills/ @jeschulz +/microsoft/ @jesperschulz +/skills/ @jesperschulz # GitHub Actions and CI -/.github/ @jeschulz +/.github/ @jesperschulz + +# Domain experts β€” required reviewers for coding rules +/microsoft/knowledge/events/ @AleksandricMarko @pchriste-microsoft-com +/microsoft/knowledge/performance/ @BardurKnudsen @pchriste-microsoft-com +/microsoft/knowledge/privacy/ @haoranpb @pchriste-microsoft-com +/microsoft/knowledge/security/ @darjoo @WaelAbuSeada @Aleyenda @pchriste-microsoft-com +/microsoft/knowledge/style/ @nikolakukrika @jesperschulz @pchriste-microsoft-com +/microsoft/knowledge/testing/ @nikolakukrika @ventselartur @pchriste-microsoft-com +/microsoft/knowledge/upgrade/ @nikolakukrika @pchriste-microsoft-com # Community content β€” open to broader review # /community/ reviewers are added as the contributor base grows diff --git a/README.md b/README.md index e7a180e..ddab523 100644 --- a/README.md +++ b/README.md @@ -1,9 +1,3 @@ -# ⚠️ Warning -This project is under active development. -Large and potentially breaking changes are expected. - -**Public preview will soon be announced.** - # BCQuality Quality skills and knowledge for Business Central development. @@ -58,7 +52,7 @@ Skills define how agents consume knowledge. They come in three flavors: READ and DO are read on demand β€” typically when the first dispatched action skill runs. They are not prerequisites for invoking Entry. WRITE is only used when scaffolding new content. -- **Action skills** β€” concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/review/al-code-review.md`](microsoft/skills/review/al-code-review.md) (super-skill), which composes six leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) β€” one per knowledge domain (performance, security, privacy, upgrade, style, UI). +- **Action skills** β€” concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/review/al-code-review.md`](microsoft/skills/review/al-code-review.md) (super-skill), which composes the AL review leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) β€” one per knowledge domain. ### Agent bootstrapping @@ -72,7 +66,7 @@ Every knowledge file is a markdown file with mandatory YAML frontmatter. Files t ```yaml --- -bc-version: [all] # or [26..28] for version-gated guidance +bc-version: [all] # or [26..28], or [26..] for "26 and later" domain: performance # security | performance | ux | telemetry | ... keywords: [query, filtering, partial] # free-text tags for retrieval technologies: [al] # al | javascript | powershell | ... diff --git a/community/knowledge/ui/default-descending-sort-on-historical-pages.bad.al b/community/knowledge/ui/default-descending-sort-on-historical-pages.bad.al new file mode 100644 index 0000000..42201ad --- /dev/null +++ b/community/knowledge/ui/default-descending-sort-on-historical-pages.bad.al @@ -0,0 +1,29 @@ +page 50100 "Integration Log Entries" +{ + PageType = List; + SourceTable = "Integration Log Entry"; + ApplicationArea = All; + UsageCategory = History; + Caption = 'Integration Log Entries'; + + // No descending default sort: the page opens oldest-first. + + layout + { + area(Content) + { + repeater(General) + { + field("Entry No."; Rec."Entry No.") + { + } + field(Status; Rec.Status) + { + } + field(Message; Rec.Message) + { + } + } + } + } +} \ No newline at end of file diff --git a/community/knowledge/ui/default-descending-sort-on-historical-pages.good.al b/community/knowledge/ui/default-descending-sort-on-historical-pages.good.al new file mode 100644 index 0000000..8d2e75b --- /dev/null +++ b/community/knowledge/ui/default-descending-sort-on-historical-pages.good.al @@ -0,0 +1,30 @@ +page 50100 "Integration Log Entries" +{ + PageType = List; + SourceTable = "Integration Log Entry"; + ApplicationArea = All; + UsageCategory = History; + Caption = 'Integration Log Entries'; + + // Historical pages should open with the newest records first. + SourceTableView = order(descending); + + layout + { + area(Content) + { + repeater(General) + { + field("Entry No."; Rec."Entry No.") + { + } + field(Status; Rec.Status) + { + } + field(Message; Rec.Message) + { + } + } + } + } +} \ No newline at end of file diff --git a/community/knowledge/ui/default-descending-sort-on-historical-pages.md b/community/knowledge/ui/default-descending-sort-on-historical-pages.md new file mode 100644 index 0000000..185e56f --- /dev/null +++ b/community/knowledge/ui/default-descending-sort-on-historical-pages.md @@ -0,0 +1,23 @@ +--- +bc-version: [all] +domain: ui +keywords: [historical-table, list-page, descending-sort, log-entry, ledger-entry, archive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Default descending sort on historical pages + +## Description +Historical list pages should default to showing the newest records first. On pages such as log entries, ledger entries, archives, and other history lists, an oldest-first default order does not align with the primary use of the page, which is typically to review recent activity. + +## Best Practice +Set descending sort as the default on list pages whose primary purpose is to present historical records. This is the expected default for entry, log, archive, and posted-history pages unless there is a specific requirement to begin with the oldest record. + +See sample: `default-descending-sort-on-historical-pages.good.al`. + +## Anti Pattern +Using an oldest-first default order on a historical list page where users are primarily interested in recent activity. Typical signs include history, log, or entry pages that regularly need to be re-sorted to descending during normal use. + +See sample: `default-descending-sort-on-historical-pages.bad.al`. \ No newline at end of file diff --git a/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.bad.al b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.bad.al new file mode 100644 index 0000000..b0cc156 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.bad.al @@ -0,0 +1,21 @@ +codeunit 50326 "Order Processor Bad" +{ + // Anti-pattern: every helper is public by default, exposing implementation + // detail as a de-facto API. Each becomes a contract that cannot be changed + // without risking breakage for consumers that bound to it. + procedure ProcessOrder(OrderNo: Code[20]) + begin + ValidateOrder(OrderNo); + PostOrder(OrderNo); + end; + + procedure ValidateOrder(OrderNo: Code[20]) + begin + if OrderNo = '' then + Error('Order number is required.'); + end; + + procedure PostOrder(OrderNo: Code[20]) + begin + end; +} diff --git a/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.good.al b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.good.al new file mode 100644 index 0000000..1b53a68 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.good.al @@ -0,0 +1,21 @@ +codeunit 50325 "Order Processor Good" +{ + // Supported, stable entry point β€” intentionally public. + procedure ProcessOrder(OrderNo: Code[20]) + begin + ValidateOrder(OrderNo); + PostOrder(OrderNo); + end; + + // In-app reuse only β€” internal, so it is not part of the external contract. + internal procedure ValidateOrder(OrderNo: Code[20]) + begin + if OrderNo = '' then + Error('Order number is required.'); + end; + + // Implementation detail confined to this object β€” local. + local procedure PostOrder(OrderNo: Code[20]) + begin + end; +} diff --git a/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md new file mode 100644 index 0000000..835312d --- /dev/null +++ b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [access-modifier, internal, local, public, protected, scope, encapsulation] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Choose access modifiers deliberately + +## Description + +Access is a decision about what you are willing to support forever. The moment a procedure or object is reachable from another extension β€” no `local`, or a removed `[Scope('OnPrem')]` β€” it becomes a contract: callers bind to it, and changing or removing it later is a breaking change. The safe default is the narrowest access that works. Use `local` for implementation detail confined to one object, `internal` for code shared within the app but not exposed to consumers, and `protected var` for state intended as an inheritance point for extension objects. Reserve `public` for the deliberate, supported entry points you intend to maintain as a stable API. LLMs tend to make everything public "to be safe," which inverts the rule and turns every helper into an accidental contract. + +## Best Practice + +Start everything `local` or `internal` and promote a member to `public` only when you have decided to support it as a stable contract. Expose a small, intentional surface β€” the supported entry point β€” and keep validation, posting, and helper routines `internal` for in-app reuse or `local` when single-object. Do not drop `[Scope('OnPrem')]` without intent, since that too widens the contract. Every public member is a maintenance commitment; spend them deliberately. + +See sample: `choose-access-modifiers-deliberately.good.al`. + +## Anti Pattern + +Declaring every procedure `public` by default, so internal helpers like `ValidateOrder` and `PostOrder` become a de-facto API that consumers bind to and that can no longer be changed freely. Detection: an object where implementation-detail procedures carry no access modifier or are `public` without a reason to support them externally. Default them to `internal`/`local` and make only the intended entry point public. + +See sample: `choose-access-modifiers-deliberately.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.bad.al b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.bad.al new file mode 100644 index 0000000..a959573 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.bad.al @@ -0,0 +1,10 @@ +codeunit 50306 "Net Amount Api Bad" +{ + // Breaking: the published CalcNet procedure was renamed outright with no + // deprecation window and no [Obsolete] marker. Every extension that called + // CalcNet breaks the instant it consumes this version. + procedure CalculateNetAmount(GrossAmount: Decimal; TaxRate: Decimal): Decimal + begin + exit(GrossAmount / (1 + TaxRate)); + end; +} diff --git a/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.good.al b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.good.al new file mode 100644 index 0000000..4f2638a --- /dev/null +++ b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.good.al @@ -0,0 +1,15 @@ +codeunit 50305 "Net Amount Api Good" +{ + // Old name kept and marked obsolete: callers still compile but get a warning + // pointing at the replacement, with a tag recording the removal target version. + [Obsolete('Use CalculateNetAmount instead.', '25.0')] + procedure CalcNet(GrossAmount: Decimal; TaxRate: Decimal): Decimal + begin + exit(CalculateNetAmount(GrossAmount, TaxRate)); + end; + + procedure CalculateNetAmount(GrossAmount: Decimal; TaxRate: Decimal): Decimal + begin + exit(GrossAmount / (1 + TaxRate)); + end; +} diff --git a/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.md b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.md new file mode 100644 index 0000000..5a699b6 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [obsolete, deprecation, obsoletestate, obsoletetag, pending, removed, public-procedure] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Deprecate public members through the Obsolete lifecycle, never delete them outright + +## Description + +Deleting or renaming a published procedure (or object) in a single release is a hard break: dependent extensions that reference it stop compiling the moment they pick up the new version, with no warning window to migrate. AL provides a staged deprecation lifecycle precisely so consumers get advance notice. For a procedure, apply the `[Obsolete('reason', 'tag')]` attribute: the member keeps working but every caller gets a compiler warning naming the replacement and the target version. The member stays through a deprecation window β€” at least one major release β€” before it is finally removed. Object- and field-level members use the matching `ObsoleteState = Pending` β†’ `Removed` property progression. LLMs trained to "clean up" code often delete or rename the old member immediately, skipping the window entirely. + +## Best Practice + +When a published procedure is superseded, keep it in place and mark it `[Obsolete('Use CalculateNetAmount instead.', '25.0')]`, where the message names the replacement and the tag records the target version for removal. Have the obsolete member forward to the new one so behavior is preserved during the window. Only after the deprecation window has elapsed β€” a later release β€” change its state to removed. This gives every dependent app a compile-time signal and time to migrate before anything actually disappears. + +See sample: `deprecate-public-members-with-the-obsolete-lifecycle.good.al`. + +## Anti Pattern + +Renaming or deleting the published `CalcNet` procedure in place β€” replacing it with `CalculateNetAmount` and nothing else β€” so consumers calling `CalcNet` break immediately with no deprecation notice. Detection: a previously shipped non-`local` procedure that vanished or was renamed between versions with no `[Obsolete]` marker left behind on a kept member. Mark it obsolete and keep it for a window instead. + +See sample: `deprecate-public-members-with-the-obsolete-lifecycle.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.bad.al b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.bad.al new file mode 100644 index 0000000..6c2f7ca --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.bad.al @@ -0,0 +1,10 @@ +codeunit 50301 "Discount Api Bad" +{ + // Breaking: a Rate parameter was added to a procedure that already shipped. + // Every dependent extension that called CalculateDiscount(Amount) now fails + // to compile until it is changed and recompiled. + procedure CalculateDiscount(Amount: Decimal; Rate: Decimal): Decimal + begin + exit(Amount * Rate); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.good.al b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.good.al new file mode 100644 index 0000000..4640a69 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.good.al @@ -0,0 +1,16 @@ +codeunit 50300 "Discount Api Good" +{ + // Published contract β€” signature kept exactly as it shipped. + procedure CalculateDiscount(Amount: Decimal): Decimal + begin + exit(Amount * 0.05); + end; + + // New capability added as a separate overload, so existing callers of + // CalculateDiscount(Amount) keep compiling. The return value is named, which + // is the one signature change that is always safe to make. + procedure CalculateDiscountWithRate(Amount: Decimal; Rate: Decimal) Discount: Decimal + begin + Discount := Amount * Rate; + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md new file mode 100644 index 0000000..a557d41 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [signature, public-procedure, parameter, return-value, overload, contract] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not change the signature of a published procedure + +## Description + +A procedure that is reachable from outside its object β€” any procedure not marked `local` (and, for on-prem-scoped code, anything a dependent app can still bind to) β€” is a contract. Once another extension compiles against it, changing its shape breaks that extension at build time. Signature changes include adding, removing, or reordering parameters, changing a parameter or return type, and toggling a parameter between by-value and `var` (by-reference). The platform treats the procedure's identity as its full signature, so even a "compatible-looking" tweak is a new method to dependents. There is exactly one safe edit: naming a previously unnamed return value, which adds no caller obligation. LLMs routinely "improve" a public procedure in place by adding a parameter, not realizing every consumer must be recompiled. + +## Best Practice + +Treat a published signature as frozen. When new behavior needs more inputs, add a new procedure or overload alongside the original β€” for example a `CalculateDiscountWithRate(Amount; Rate)` next to the unchanged `CalculateDiscount(Amount)` β€” and let the old one delegate to the new one. Existing callers keep compiling; new callers opt into the richer entry point. Naming an unnamed return value is the one in-place change that is always safe. + +See sample: `do-not-change-published-procedure-signatures.good.al`. + +## Anti Pattern + +Editing the existing public procedure's parameter list β€” here, adding a `Rate` parameter to `CalculateDiscount` β€” so every dependent extension that called the old form fails to compile. Detection: a parameter added, removed, reordered, retyped, or flipped to/from `var`, or a changed return type, on any non-`local` procedure that already shipped. Add a new overload instead. + +See sample: `do-not-change-published-procedure-signatures.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.bad.al b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.bad.al new file mode 100644 index 0000000..e9d0941 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.bad.al @@ -0,0 +1,13 @@ +codeunit 50321 "Payment Client Bad" +{ + var + AccessToken: Text; + + // Crossing the trust boundary: a public getter hands the raw credential to any + // caller, turning a secret into a de-facto public API that cannot be removed + // later without breaking consumers. + procedure GetAccessToken(): Text + begin + exit(AccessToken); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.good.al b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.good.al new file mode 100644 index 0000000..4073930 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.good.al @@ -0,0 +1,25 @@ +codeunit 50320 "Payment Client Good" +{ + var + AccessToken: Text; + + // Credential flows inward through an internal setter and never leaves the object. + internal procedure SetAccessToken(NewToken: Text) + begin + AccessToken := NewToken; + end; + + // Public API exposes only non-sensitive data β€” a masked reference, never the token. + procedure GetMaskedReference(): Text + var + Reference: Text; + begin + Reference := LastReference(); + exit('****-' + CopyStr(Reference, StrLen(Reference) - 3)); + end; + + local procedure LastReference(): Text + begin + exit('REF000123456'); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.md b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.md new file mode 100644 index 0000000..65c7971 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [sensitive-data, secrettext, token, credential, public-api, access-boundary] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not widen access to expose sensitive data through a public API + +## Description + +Every member you make publicly reachable becomes a contract you must keep β€” and when that member returns a secret, the contract leaks the secret. Widening access to a credential happens in several shapes: a public getter that returns a raw token or password, an event whose parameter carries a secret to every subscriber, or a global variable holding a key that an extension can read. Once such a surface ships, removing it is itself a breaking change, so the exposure is hard to walk back. Sensitive material β€” tokens, passwords, connection secrets, `SecretText` values, security internals β€” must stay inside `internal` or `local` members. Public surfaces should expose only non-sensitive business data. LLMs often add a convenient `GetToken()` getter without recognizing it as a permanent security boundary breach. + +## Best Practice + +Keep secrets in `internal` or `local` members, and prefer the `SecretText` type so the value cannot be read back or logged. Where callers genuinely need a credential, pass it inward (a setter) rather than handing it outward (a getter). Public API should return only non-sensitive data β€” a masked reference, a status, a business identifier β€” never the raw secret. Treat each public member as a lasting commitment and keep the security-sensitive surface as small as possible. + +See sample: `do-not-expose-sensitive-data-through-public-api.good.al`. + +## Anti Pattern + +A public `GetAccessToken()` that returns the raw token (or an event parameter carrying a credential to all subscribers), turning a secret into a de-facto public API any dependent can consume. Detection: a non-`local` procedure, event parameter, or global variable that surfaces a token, password, key, or other credential. Keep the secret internal and expose only non-sensitive data. + +See sample: `do-not-expose-sensitive-data-through-public-api.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.bad.al b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.bad.al new file mode 100644 index 0000000..c3727a9 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.bad.al @@ -0,0 +1,22 @@ +codeunit 50316 "Pricing Api Bad" +{ + // Anti-pattern: new surcharge logic is added inside a procedure already marked + // obsolete, and inside a #if not CLEAN25 block. Both are scheduled for removal, + // so this behaviour disappears the moment CLEAN25 is enabled. + [Obsolete('Use GetUnitPrice instead.', '25.0')] + procedure GetPrice(ItemNo: Code[20]): Decimal + var + Price: Decimal; + begin + Price := 100; +#if not CLEAN25 + Price += CalculateSurcharge(ItemNo); +#endif + exit(Price); + end; + + local procedure CalculateSurcharge(ItemNo: Code[20]): Decimal + begin + exit(5); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.good.al b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.good.al new file mode 100644 index 0000000..0049068 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.good.al @@ -0,0 +1,26 @@ +codeunit 50315 "Pricing Api Good" +{ + // Obsolete member left untouched β€” it only forwards to the replacement and + // gains no new logic. + [Obsolete('Use GetUnitPrice instead.', '25.0')] + procedure GetPrice(ItemNo: Code[20]): Decimal + begin + exit(GetUnitPrice(ItemNo)); + end; + + // New behaviour is built on the supported replacement, not on the obsolete member. + procedure GetUnitPrice(ItemNo: Code[20]): Decimal + begin + exit(CalculateBasePrice(ItemNo) + CalculateSurcharge(ItemNo)); + end; + + local procedure CalculateBasePrice(ItemNo: Code[20]): Decimal + begin + exit(100); + end; + + local procedure CalculateSurcharge(ItemNo: Code[20]): Decimal + begin + exit(5); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.md b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.md new file mode 100644 index 0000000..781810b --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [obsolete, clean-flag, conditional-compilation, deprecation, replacement, do-not-extend] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not build on code already marked obsolete + +## Description + +A member carrying `[Obsolete]`, or wrapped in a `#if not CLEANxx` conditional-compilation block, is already scheduled for deletion β€” the `CLEANxx` symbol is flipped on in a future release to strip that code out. Adding logic, raising new events, or taking fresh dependencies on such a member ties live behavior to something the platform is about to remove. When the deprecation completes, everything layered on top breaks. The obsolete marker is a one-way signal: it means "migrate off," never "safe to extend." LLMs frequently edit whatever procedure is nearest to the change, including obsolete ones, and add `#if not CLEANxx` branches without understanding that the block is transient. + +## Best Practice + +Leave obsolete members exactly as they are and implement against the current, supported replacement. New logic β€” a surcharge calculation, an event publisher, a hook β€” belongs on the live API (`GetUnitPrice`), never inside the deprecated `GetPrice` or behind a `#if not CLEAN25` guard. If the replacement does not yet exist, create it as a first-class member and build there. The obsolete code should only shrink over time, not accrete new behavior. + +See sample: `do-not-modify-code-already-marked-obsolete.good.al`. + +## Anti Pattern + +Adding a surcharge calculation inside the `[Obsolete]` `GetPrice` procedure, or behind a `#if not CLEAN25` block, so the new behavior is wired to code that will be removed when `CLEAN25` is enabled. Detection: new statements, event declarations, or dependencies introduced inside an `[Obsolete]`-marked member or a `#if not CLEANxx` region. Move the logic onto the supported replacement instead. + +See sample: `do-not-modify-code-already-marked-obsolete.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al new file mode 100644 index 0000000..0e2f000 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al @@ -0,0 +1,11 @@ +table 50311 "Customer Profile Bad" +{ + fields + { + field(1; "No."; Code[20]) { } + // Breaking: the published "Email" field was renamed in place. Dependent + // extensions that reference "Email" stop compiling, and the data stored in + // the old column is orphaned on upgrade. + field(2; "Contact Email"; Text[80]) { } + } +} diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al new file mode 100644 index 0000000..ed239d2 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al @@ -0,0 +1,17 @@ +table 50310 "Customer Profile Good" +{ + fields + { + field(1; "No."; Code[20]) { } + // Replacement field shipped alongside the old one. + field(2; "Contact Email"; Text[80]) { } + // Old field kept and marked Pending so dependent code keeps compiling and + // an upgrade codeunit can copy its data before it is finally removed. + field(3; "Email"; Text[80]) + { + ObsoleteState = Pending; + ObsoleteReason = 'Replaced by Contact Email. Will be removed after the deprecation window.'; + ObsoleteTag = '25.0'; + } + } +} diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md new file mode 100644 index 0000000..e1e7116 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [table-field, obsoletestate, obsoletereason, obsoletetag, pending, removed, data-loss] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Obsolete published table fields instead of deleting or renaming them + +## Description + +A table field that has shipped carries two contracts at once: extensions reference it by name, and the database holds data in its column. Deleting the field, or renaming it (which the platform treats as drop-plus-add), breaks dependent code at compile time and discards the stored data β€” a silent data-loss event on upgrade. The fix is the same staged lifecycle used for objects: set `ObsoleteState = Pending` together with `ObsoleteReason` and an `ObsoleteTag` naming the target version, ship the new field alongside, migrate data during the window, and only switch the old field to `ObsoleteState = Removed` in a later release once nothing depends on it. LLMs often "tidy" a schema by renaming a field in place, not realizing this is both a breaking change and a data-loss risk. + +## Best Practice + +Add the replacement field, then mark the old field `ObsoleteState = Pending` with an `ObsoleteReason` that names the replacement and an `ObsoleteTag` carrying the target version (for example `'25.0'`). Keep the obsolete field readable so an upgrade codeunit can copy its data into the new field during the deprecation window. Move it to `ObsoleteState = Removed` only in a later major version, after the window has passed and data has migrated. + +See sample: `obsolete-table-fields-instead-of-deleting-them.good.al`. + +## Anti Pattern + +Renaming the published `Email` field to `Contact Email` directly in the table β€” or deleting it β€” so dependent extensions that reference `Email` break and the column's stored values are orphaned on upgrade. Detection: a previously shipped field removed or renamed in a table or table extension with no `ObsoleteState = Pending` step preserving the original. Obsolete the field through the lifecycle instead. + +See sample: `obsolete-table-fields-instead-of-deleting-them.bad.al`. diff --git a/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.bad.al b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.bad.al new file mode 100644 index 0000000..cdb5fa9 --- /dev/null +++ b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.bad.al @@ -0,0 +1,21 @@ +codeunit 50187 "Collect Errors Bad Sample" +{ + procedure ValidateAllItems() + var + Item: Record Item; + ErrorText: Text; + begin + // Hand-rolled accumulation: reimplements the platform feature, loses each + // error's ErrorInfo structure, and skips telemetry classification. + if Item.FindSet() then + repeat + if Item.Description = '' then + ErrorText += StrSubstNo('Item %1 has no description.\', Item."No."); + if Item."Unit Cost" <= 0 then + ErrorText += StrSubstNo('Item %1 must have a positive unit cost.\', Item."No."); + until Item.Next() = 0; + + if ErrorText <> '' then + Error(ErrorText); + end; +} diff --git a/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.good.al b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.good.al new file mode 100644 index 0000000..dcd64b9 --- /dev/null +++ b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.good.al @@ -0,0 +1,37 @@ +codeunit 50185 "Collect Errors Good Sample" +{ + [ErrorBehavior(ErrorBehavior::Collect)] + procedure ValidateAllItems() + var + Item: Record Item; + CollectedErrors: List of [ErrorInfo]; + CollectedError: ErrorInfo; + ErrorText: Text; + begin + if Item.FindSet() then + repeat + // Run each item in its own context so one failure does not abandon the rest. + Codeunit.Run(Codeunit::"Collect Errors Item Check", Item); + until Item.Next() = 0; + + if HasCollectedErrors() then begin + CollectedErrors := GetCollectedErrors(); + foreach CollectedError in CollectedErrors do + ErrorText += CollectedError.Message() + '\'; + Message('The following must be fixed before posting:\%1', ErrorText); + end; + end; +} + +codeunit 50186 "Collect Errors Item Check" +{ + TableNo = Item; + + trigger OnRun() + begin + if Rec.Description = '' then + Error('Item %1 has no description.', Rec."No."); + if Rec."Unit Cost" <= 0 then + Error('Item %1 must have a positive unit cost.', Rec."No."); + end; +} diff --git a/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.md b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.md new file mode 100644 index 0000000..6cc891c --- /dev/null +++ b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [collectible-errors, errorbehavior, collect, getcollectederrors, hascollectederrors, validation, batch] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Collect validation errors with ErrorBehavior::Collect and handle the collected list + +## Description + +By default a procedure stops on the first `Error`, so a user fixing ten bad rows must rerun the operation ten times. The collectible-errors feature postpones error handling to the end of the call: a procedure attributed `[ErrorBehavior(ErrorBehavior::Collect)]` keeps running as errors occur and gathers them, so all failures can be presented together. The collected errors are read with `HasCollectedErrors()` and `GetCollectedErrors()` (which returns a `List of [ErrorInfo]`); `ClearCollectedErrors()` empties the buffer. This is a platform mechanism most LLMs are unaware of β€” they reach for a manually concatenated `Text` buffer or a temporary error table instead. + +## Best Practice + +Mark the orchestrating procedure `[ErrorBehavior(ErrorBehavior::Collect)]` and run each item's validation so one failure doesn't abandon the rest β€” typically by calling the per-item routine through `Codeunit.Run`. When the run finishes, inspect `HasCollectedErrors()` and surface `GetCollectedErrors()` to the user as a single list. Always handle the collected errors yourself: the platform's own guidance is that any errors still in the collected list when the procedure ends are concatenated into one dialog, which is hard for users to read. + +See sample: `collect-validation-errors-with-errorbehavior.good.al`. + +## Anti Pattern + +Two shapes signal trouble. The first is hand-rolled accumulation β€” appending messages to a `Text` variable and showing them at the end β€” which reimplements the platform feature, loses each error's `ErrorInfo` structure, and skips telemetry classification. The second is applying `[ErrorBehavior(ErrorBehavior::Collect)]` but never calling `HasCollectedErrors`/`GetCollectedErrors`, so every collected error spills into the platform's concatenated end-of-procedure dialog. Detection: a `Collect` attribute with no matching `GetCollectedErrors` call, or a per-row loop that builds an error string by concatenation. + +See sample: `collect-validation-errors-with-errorbehavior.bad.al`. diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.bad.al b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.bad.al new file mode 100644 index 0000000..4e6be07 --- /dev/null +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.bad.al @@ -0,0 +1,14 @@ +codeunit 50191 "Error Type Bad Sample" +{ + procedure ApplyLedgerBucket(BucketId: Integer) + begin + // Developer-facing detail shown straight to the user, and no structured telemetry signal. + if not BucketInitialized(BucketId) then + Error('Unexpected state: ledger bucket %1 not initialized', BucketId); + end; + + local procedure BucketInitialized(BucketId: Integer): Boolean + begin + exit(false); + end; +} diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al new file mode 100644 index 0000000..9791909 --- /dev/null +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al @@ -0,0 +1,19 @@ +codeunit 50190 "Error Type Good Sample" +{ + procedure ApplyLedgerBucket(BucketId: Integer) + var + InternalErr: ErrorInfo; + begin + if not BucketInitialized(BucketId) then begin + InternalErr.ErrorType := ErrorType::Internal; + InternalErr.Message := StrSubstNo('Ledger bucket %1 was not initialized before posting.', BucketId); + InternalErr.DetailedMessage := 'Internal invariant violated. Inspect the call stack captured in telemetry.'; + Error(InternalErr); + end; + end; + + local procedure BucketInitialized(BucketId: Integer): Boolean + begin + exit(false); + end; +} diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md new file mode 100644 index 0000000..7264ad6 --- /dev/null +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [errorinfo, errortype, internal, client, telemetry, diagnostics, generic-message] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Set ErrorInfo.ErrorType to Internal for defects you want in telemetry but not in the user's face + +## Description + +`ErrorInfo.ErrorType` controls where an error's message is shown. With `ErrorType::Client` β€” the behaviour of a normal `Error` β€” the message is both shown to the user and sent to telemetry. With `ErrorType::Internal` the user sees a generic message while the specific message you set is sent to telemetry only. The distinction matters for *unexpected* failures β€” a broken invariant, a failed internal assertion, a "this should never happen" branch β€” where the technical detail helps the partner diagnose the defect but would only confuse the end user. LLMs are unaware `ErrorType` exists, so they expose raw internal-failure text directly to users. + +## Best Practice + +Reserve `ErrorType::Internal` for errors the user cannot act on: corrupted internal state, an unreachable branch, a contract a caller violated. Set a precise, detail-rich `Message` and `DetailedMessage` for telemetry, raise it via `Error(ErrorInfo)`, and let the platform show the user a generic dialog. Keep `ErrorType::Client` (or a plain `Error`) for failures the user is expected to read and resolve β€” validation messages, missing setup, business-rule violations. The test is simple: if the message only makes sense to a developer, mark it `Internal`. + +See sample: `errortype-internal-vs-client-for-diagnostics.good.al`. + +## Anti Pattern + +Raising an internal failure with a plain `Error('Unexpected state: ledger bucket %1 not initialized', BucketId)`. The user is shown a technical message they can do nothing about, and the signal is buried in a generic error rather than carried as structured telemetry detail. Detection: an `Error` whose wording targets a developer ("unexpected", "should not happen", raw internal identifiers) raised with default `Client` visibility instead of an `ErrorInfo` marked `ErrorType::Internal`. + +See sample: `errortype-internal-vs-client-for-diagnostics.bad.al`. diff --git a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.bad.al b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.bad.al new file mode 100644 index 0000000..bfd624a --- /dev/null +++ b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.bad.al @@ -0,0 +1,21 @@ +table 50182 "Actionable Error Bad Sample" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Qty. to Invoice"; Decimal) + { + trigger OnValidate() + begin + // Dead-end error: the code knows the maximum but offers the user no way to apply it. + if "Qty. to Invoice" > MaxQtyToInvoice() then + Error('You cannot invoice more than %1 units.', MaxQtyToInvoice()); + end; + } + } + + local procedure MaxQtyToInvoice(): Decimal + begin + exit(10); + end; +} diff --git a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.good.al b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.good.al new file mode 100644 index 0000000..8880743 --- /dev/null +++ b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.good.al @@ -0,0 +1,44 @@ +table 50180 "Actionable Error Good Sample" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Qty. to Invoice"; Decimal) + { + trigger OnValidate() + var + CannotInvoiceErr: ErrorInfo; + begin + if "Qty. to Invoice" > MaxQtyToInvoice() then begin + CannotInvoiceErr.Title := 'Qty. to Invoice isn''t valid'; + CannotInvoiceErr.Message := StrSubstNo('You cannot invoice more than %1 units.', MaxQtyToInvoice()); + CannotInvoiceErr.DetailedMessage := 'Reduce the quantity to invoice, or apply the maximum allowed.'; + CannotInvoiceErr.RecordId := Rec.RecordId(); + CannotInvoiceErr.AddAction( + StrSubstNo('Set value to %1', MaxQtyToInvoice()), + Codeunit::"Actionable Error Fixit Sample", + 'SetQtyToMax'); + Error(CannotInvoiceErr); + end; + end; + } + } + + local procedure MaxQtyToInvoice(): Decimal + begin + exit(10); + end; +} + +codeunit 50181 "Actionable Error Fixit Sample" +{ + procedure SetQtyToMax(SourceError: ErrorInfo) + var + Line: Record "Actionable Error Good Sample"; + begin + if Line.Get(SourceError.RecordId) then begin + Line.Validate("Qty. to Invoice", 10); + Line.Modify(true); + end; + end; +} diff --git a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md new file mode 100644 index 0000000..f774cc8 --- /dev/null +++ b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md @@ -0,0 +1,26 @@ +--- +bc-version: [23..] +domain: error-handling +keywords: [errorinfo, actionable-errors, fix-it, show-it, addaction, addnavigationaction, error-dialog] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Prefer ErrorInfo with recommended actions over a plain Error for recoverable failures + +## Description + +A plain `Error('text')` ends the operation with a dead-end dialog: the user reads the message but the system offers no way forward. The `ErrorInfo` data type, combined with the actionable-errors framework added in 2023 release wave 2, lets an error carry a recommended action the user can take to unblock themselves without leaving their task. Two kinds exist: a **Fix-it** action (`AddAction`), used when the code already knows the correct value and can apply it in one step, and a **Show-it** action (`AddNavigationAction` together with `PageNo`), used when the correction lives on a related record the user should be taken to. An error dialog renders at most two recommended actions. LLMs trained on older AL almost always emit a bare `Error(...)` and rarely reach for `ErrorInfo`, so this guidance is remedial. + +## Best Practice + +Build an `ErrorInfo`, set `Title`, `Message`, and `DetailedMessage`, then attach the action that matches the situation. For a Fix-it, call `AddAction(Caption, Codeunit::Handler, 'MethodName')` where the handler method (which receives the `ErrorInfo`) applies the known-good value; phrase the caption as "Set value to …". For a Show-it, set `PageNo := Page::"…"`, set `RecordId` so navigation opens the right record, and call `AddNavigationAction('Show …')`. Raise it with `Error(ErrorInfo)`. Reserve recommended actions for cases where the solution is genuinely known and the user has permission to apply it. + +See sample: `prefer-errorinfo-for-actionable-errors.good.al`. + +## Anti Pattern + +Surfacing a recoverable validation failure with `Error('You cannot invoice more than %1 units.', MaxQty)` and nothing else. The user is blocked with no offered remedy even though the code knows the maximum and could set it. The detection signal: an `Error` call in a validation or posting path whose message names a specific correct value or a specific related page, with no surrounding `ErrorInfo`, `AddAction`, or `AddNavigationAction`. Replace it with an `ErrorInfo` that carries the corresponding Fix-it or Show-it action. + +See sample: `prefer-errorinfo-for-actionable-errors.bad.al`. diff --git a/microsoft/knowledge/events/add-new-event-parameters-at-the-end.bad.al b/microsoft/knowledge/events/add-new-event-parameters-at-the-end.bad.al new file mode 100644 index 0000000..19ca979 --- /dev/null +++ b/microsoft/knowledge/events/add-new-event-parameters-at-the-end.bad.al @@ -0,0 +1,21 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50251 "Param Append Bad Sample" +{ + procedure PostDocument(var SalesHeader: Record "Sales Header"; CalledFromBatch: Boolean) + var + IsHandled: Boolean; + begin + IsHandled := false; + // Anti-pattern: 'CalledFromBatch' was inserted before the existing + // IsHandled parameter, shifting it and breaking the argument positions + // every existing subscriber relied on. + OnBeforePostDocument(SalesHeader, CalledFromBatch, IsHandled); + if IsHandled then + exit; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforePostDocument(var SalesHeader: Record "Sales Header"; CalledFromBatch: Boolean; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/add-new-event-parameters-at-the-end.good.al b/microsoft/knowledge/events/add-new-event-parameters-at-the-end.good.al new file mode 100644 index 0000000..8a13087 --- /dev/null +++ b/microsoft/knowledge/events/add-new-event-parameters-at-the-end.good.al @@ -0,0 +1,20 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50250 "Param Append Good Sample" +{ + procedure PostDocument(var SalesHeader: Record "Sales Header"; CalledFromBatch: Boolean) + var + IsHandled: Boolean; + begin + IsHandled := false; + // The new 'CalledFromBatch' parameter was appended at the end of the + // existing signature, so existing subscribers needed no re-mapping. + OnBeforePostDocument(SalesHeader, IsHandled, CalledFromBatch); + if IsHandled then + exit; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforePostDocument(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean; CalledFromBatch: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/add-new-event-parameters-at-the-end.md b/microsoft/knowledge/events/add-new-event-parameters-at-the-end.md new file mode 100644 index 0000000..1f1dc14 --- /dev/null +++ b/microsoft/knowledge/events/add-new-event-parameters-at-the-end.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [event-parameters, signature, backward-compatibility, append, onbefore, integration-event, versioning] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Add new event parameters at the end + +## Description + +Adding a parameter to an existing event publisher changes its signature. Appending the new parameter at the end of the parameter list keeps the change easy to review and track: existing subscribers still bind to the leading parameters, and the diff is a single clean addition. Inserting a parameter in the middle makes diffs noisy and harder to review, and obscures the history of how the signature evolved. New parameters belong after the existing ones. + +## Best Practice + +When extending an existing publisher, append the new parameter after all existing ones, including after a trailing `var IsHandled: Boolean` when present. Subscribers that already match keep working against the leading parameters, and the change stays a one-line addition that is trivial to review. + +See sample: `add-new-event-parameters-at-the-end.good.al`. + +## Anti Pattern + +Inserting a new parameter in the middle of an existing event's signature, shifting every subsequent parameter and making the change noisy and harder to review. Detection: a changed event signature where an added parameter appears before existing parameters rather than at the tail of the list. + +See sample: `add-new-event-parameters-at-the-end.bad.al`. diff --git a/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.bad.al b/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.bad.al new file mode 100644 index 0000000..dcf92a1 --- /dev/null +++ b/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.bad.al @@ -0,0 +1,18 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50286 "Typed Param Bad Sample" +{ + procedure ValidateQuantity(var SalesLine: Record "Sales Line"; xSalesLine: Record "Sales Line") + var + RecRef: RecordRef; + begin + // Anti-pattern: a RecordRef drops the table type and xRec is ambiguous + // out of context, so subscribers lose type safety and a clear contract. + RecRef.GetTable(SalesLine); + OnAfterValidateQuantity(RecRef, xSalesLine); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterValidateQuantity(var RecRef: RecordRef; xSalesLine: Record "Sales Line") + begin + end; +} diff --git a/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.good.al b/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.good.al new file mode 100644 index 0000000..abb5f0d --- /dev/null +++ b/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.good.al @@ -0,0 +1,14 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50285 "Typed Param Good Sample" +{ + procedure ValidateQuantity(var SalesLine: Record "Sales Line"; PreviousQuantity: Decimal) + begin + // A concrete record plus the specific value needed: type-safe contract. + OnAfterValidateQuantity(SalesLine, PreviousQuantity); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterValidateQuantity(var SalesLine: Record "Sales Line"; PreviousQuantity: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.md b/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.md new file mode 100644 index 0000000..6c1e004 --- /dev/null +++ b/microsoft/knowledge/events/avoid-loosely-typed-event-parameters.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [recordref, xrec, type-safety, event-parameters, strong-typing, integration-event, clarity] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Avoid loosely typed event parameters + +## Description + +Passing `RecordRef` or `xRec` as event parameters weakens the contract. A `RecordRef` parameter erases the table type, so subscribers must inspect at run time which table they received and can be handed an unexpected one, losing compile-time checking and direct field access. `xRec` β€” the previous version of a record β€” is context-dependent: it is meaningful inside a specific table or page trigger, but ambiguous once passed around as a parameter, and is often stale or empty outside the context that produced it. Prefer a concrete, strongly-typed record plus the specific values a subscriber actually needs, so the contract is explicit and the compiler enforces it. + +## Best Practice + +Give events concrete record types and explicit values, such as `(SalesLine: Record "Sales Line"; PreviousQuantity: Decimal)`, instead of a `RecordRef` or an `xRec` parameter. Subscribers then get type safety, field access, and an unambiguous contract. + +See sample: `avoid-loosely-typed-event-parameters.good.al`. + +## Anti Pattern + +Event parameters typed as `RecordRef` (no table type) or an `xRec`-style "previous record" (ambiguous, possibly stale) without strong justification. Detection: an event signature containing a `RecordRef` parameter, or a passed-through `xRec` record, where a concrete typed record and explicit values would serve. + +See sample: `avoid-loosely-typed-event-parameters.bad.al`. diff --git a/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.bad.al b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.bad.al new file mode 100644 index 0000000..4dfce97 --- /dev/null +++ b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.bad.al @@ -0,0 +1,38 @@ +// Demonstration only. Shows the wrong pattern: raising the integration event inside a TryFunction body. + +codeunit 50116 "Payment Processor Bad" +{ + [IntegrationEvent(false, false)] + procedure OnBeforeSubmitPayment(var PaymentAmount: Decimal; var Cancel: Boolean) + begin + end; + + procedure SubmitPayment(PaymentAmount: Decimal) + var + Success: Boolean; + begin + // TryFunction wraps both the event raise and the gateway call. + Success := TrySubmitPaymentInternal(PaymentAmount); + if not Success then + Error('Payment gateway call failed. Check connectivity and retry.'); + end; + + [TryFunction] + local procedure TrySubmitPaymentInternal(PaymentAmount: Decimal) + var + Cancel: Boolean; + Client: HttpClient; + Response: HttpResponseMessage; + begin + Cancel := false; + // BAD: event raised inside TryFunction. Any Error() thrown by a subscriber is caught here + // and silently swallowed - the subscriber's error never reaches the caller. + // A subscriber setting Cancel := true is also lost when TryFunction returns false. + OnBeforeSubmitPayment(PaymentAmount, Cancel); + if Cancel then + exit; + Client.Get('https://payments.example.com/submit?amount=' + Format(PaymentAmount), Response); + if not Response.IsSuccessStatusCode() then + Error('HTTP %1', Response.HttpStatusCode()); + end; +} diff --git a/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.good.al b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.good.al new file mode 100644 index 0000000..7c76303 --- /dev/null +++ b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.good.al @@ -0,0 +1,38 @@ +// Demonstration only. Shows the correct pattern: raise the integration event before entering TryFunction. + +codeunit 50114 "Payment Processor" +{ + [IntegrationEvent(false, false)] + procedure OnBeforeSubmitPayment(var PaymentAmount: Decimal; var Cancel: Boolean) + begin + end; + + procedure SubmitPayment(PaymentAmount: Decimal) + var + Cancel: Boolean; + Success: Boolean; + begin + Cancel := false; + // Event raised outside the try scope - subscriber errors propagate normally to the caller. + OnBeforeSubmitPayment(PaymentAmount, Cancel); + if Cancel then + exit; + + // Only the operation that can fail transiently lives inside TryFunction. + Success := TryCallPaymentGateway(PaymentAmount); + if not Success then + Error('Payment gateway call failed. Check connectivity and retry.'); + end; + + [TryFunction] + local procedure TryCallPaymentGateway(PaymentAmount: Decimal) + var + Client: HttpClient; + Response: HttpResponseMessage; + begin + // ... build request, set headers ... + Client.Get('https://payments.example.com/submit?amount=' + Format(PaymentAmount), Response); + if not Response.IsSuccessStatusCode() then + Error('HTTP %1', Response.HttpStatusCode()); + end; +} diff --git a/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.md b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.md new file mode 100644 index 0000000..7e791fe --- /dev/null +++ b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [tryfunction, integration-event, subscriber, error-handling, silent-failure, event-publisher] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not raise integration events inside a TryFunction + +## Description + +A `TryFunction` catches all errors β€” including errors thrown by event subscribers. When an `[IntegrationEvent]` is raised inside a `TryFunction` body, any error a subscriber raises is silently swallowed by the TryFunction's error boundary. The subscriber's logic fails, the caller sees no error, and the calling code continues as if nothing happened. Subscribers have no way to signal failure to the caller. + +## Best Practice + +Raise the integration event before entering the TryFunction scope. The event and its subscribers execute outside the error boundary, so subscriber errors propagate normally to the caller. Move only the operation that genuinely needs error isolation (such as an HTTP call or a posting step) inside the TryFunction. + +See sample: `avoid-raising-events-inside-try-functions.good.al`. + +## Anti Pattern + +Raising an integration event inside a TryFunction body. Subscriber failures are caught and discarded by the TryFunction. The subscriber contract β€” that a subscriber can signal failure to the caller β€” is silently broken. + +See sample: `avoid-raising-events-inside-try-functions.bad.al`. diff --git a/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.bad.al b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.bad.al new file mode 100644 index 0000000..4e8ecc6 --- /dev/null +++ b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.bad.al @@ -0,0 +1,53 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50232 "Order Event Pub Bad Sample" +{ + procedure ReleaseOrder(OrderNo: Code[20]) + begin + OnAfterReleaseOrder(OrderNo); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterReleaseOrder(OrderNo: Code[20]) + begin + end; +} + +// Anti-pattern 1: a static subscriber drives an always-on side effect that +// should be scoped. Every release now emails the customer, in every session +// and every automated test, with no way to switch it off. +codeunit 50233 "Always Email Sub Bad Sample" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Order Event Pub Bad Sample", 'OnAfterReleaseOrder', '', false, false)] + local procedure SendEmailOnRelease(OrderNo: Code[20]) + begin + // Send a confirmation email unconditionally on every release. + end; +} + +codeunit 50234 "Scoped Sub Bad Sample" +{ + EventSubscriberInstance = Manual; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Order Event Pub Bad Sample", 'OnAfterReleaseOrder', '', false, false)] + local procedure OverrideRelease(OrderNo: Code[20]) + begin + // Scoped behaviour intended only for a specific flow. + end; +} + +// Anti-pattern 2: a manual subscriber is bound and never unbound. Because the +// instance is held on a SingleInstance global, the binding lives for the whole +// session, so later unrelated releases keep hitting the scoped subscriber. +codeunit 50235 "Leaky Binder Bad Sample" +{ + SingleInstance = true; + + var + Scoped: Codeunit "Scoped Sub Bad Sample"; + + procedure ActivateOverride() + begin + BindSubscription(Scoped); + // Missing: a matching UnbindSubscription(Scoped) when the scope ends. + end; +} diff --git a/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.good.al b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.good.al new file mode 100644 index 0000000..66e416b --- /dev/null +++ b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.good.al @@ -0,0 +1,52 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50228 "Item Post Pub Good Sample" +{ + procedure PostItemLine(ItemNo: Code[20]; Qty: Decimal) + begin + // ... post the line ... + OnAfterPostItemLine(ItemNo, Qty); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterPostItemLine(ItemNo: Code[20]; Qty: Decimal) + begin + end; +} + +codeunit 50229 "Item Post Audit Good Sample" +{ + // Always-on behaviour belongs in a static subscriber (the default). + EventSubscriberInstance = StaticAutomatic; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Item Post Pub Good Sample", 'OnAfterPostItemLine', '', false, false)] + local procedure LogPostedLine(ItemNo: Code[20]; Qty: Decimal) + begin + // Audit every posted line, unconditionally. + end; +} + +codeunit 50230 "Item Post Stub Good Sample" +{ + // Scoped/temporary behaviour belongs in a manual subscriber. + EventSubscriberInstance = Manual; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Item Post Pub Good Sample", 'OnAfterPostItemLine', '', false, false)] + local procedure CaptureForTest(ItemNo: Code[20]; Qty: Decimal) + begin + // Record the call so a single test can assert on it. + end; +} + +codeunit 50231 "Item Post Test Good Sample" +{ + procedure VerifyPostingRaisesEvent() + var + Publisher: Codeunit "Item Post Pub Good Sample"; + Stub: Codeunit "Item Post Stub Good Sample"; + begin + // Activate the scoped subscriber only for the duration of the test. + BindSubscription(Stub); + Publisher.PostItemLine('1000', 5); + UnbindSubscription(Stub); + end; +} diff --git a/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md new file mode 100644 index 0000000..9fe7312 --- /dev/null +++ b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [event-subscriber, static-subscriber, manual-subscriber, bindsubscription, unbindsubscription, eventsubscriberinstance, scoped-binding] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Choose static vs manual subscribers deliberately and bind manual ones with BindSubscription + +## Description + +An `[EventSubscriber]` codeunit is static by default (`EventSubscriberInstance = StaticAutomatic`): it is always bound, so it fires for every raise of the event in every session. That is correct for always-on behaviour such as auditing, but wrong for behaviour that must be scoped β€” test isolation, a one-off migration, or a conditional override β€” because a static subscriber cannot be switched off. For scoped behaviour, set `EventSubscriberInstance = Manual` and activate the codeunit only while needed with `BindSubscription`, releasing it with `UnbindSubscription`. LLMs are largely unaware the manual model exists and default everything to static, producing always-on side effects that leak across unrelated operations and tests. + +## Best Practice + +Use a static subscriber for behaviour that genuinely applies all the time. For anything scoped, mark the codeunit `EventSubscriberInstance = Manual`, call `BindSubscription(SubscriberInstance)` at the start of the scope and `UnbindSubscription(SubscriberInstance)` at the end. A manual subscriber held only in a local variable unbinds automatically when that variable leaves scope, which suits test setup/teardown; a binding you intend to outlive a single call must be unbound explicitly. Keep subscriber methods `local` per CodeCop AA0207. + +See sample: `choose-static-vs-manual-subscribers-deliberately.good.al`. + +## Anti Pattern + +Two shapes. First, a static subscriber used for behaviour that should be scoped β€” an always-on side effect (sending mail, writing extra records) that now fires for every event in every session and test with no way to disable it. Second, a manual subscriber that is bound with `BindSubscription` and never unbound: when the instance is held beyond the intended scope (for example on a `SingleInstance` codeunit), the binding leaks for the whole session and later unrelated operations keep hitting it. Detection: scoped side effects on a static subscriber, or a `BindSubscription` call with no matching `UnbindSubscription` and no scope that releases the instance. + +See sample: `choose-static-vs-manual-subscribers-deliberately.bad.al`. diff --git a/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.bad.al b/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.bad.al new file mode 100644 index 0000000..8c93c25 --- /dev/null +++ b/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.bad.al @@ -0,0 +1,21 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50291 "New OnBefore Bad Sample" +{ + procedure CalculateTotal(var SalesHeader: Record "Sales Header") + var + Total: Decimal; + IsHandled: Boolean; + begin + Total := 100; + + // Anti-pattern: IsHandled was bolted onto the existing + // OnAfterCalculateTotal, changing its contract and breaking every + // subscriber that matched the original signature. + OnAfterCalculateTotal(SalesHeader, Total, IsHandled); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterCalculateTotal(var SalesHeader: Record "Sales Header"; Total: Decimal; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.good.al b/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.good.al new file mode 100644 index 0000000..47c4e67 --- /dev/null +++ b/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.good.al @@ -0,0 +1,28 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50290 "New OnBefore Good Sample" +{ + procedure CalculateTotal(var SalesHeader: Record "Sales Header") + var + Total: Decimal; + IsHandled: Boolean; + begin + // New overridable seam added as a separate event; the existing + // OnAfterCalculateTotal keeps its original signature and subscribers. + IsHandled := false; + OnBeforeCalculateTotal(SalesHeader, IsHandled); + if not IsHandled then + Total := 100; + + OnAfterCalculateTotal(SalesHeader, Total); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeCalculateTotal(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterCalculateTotal(var SalesHeader: Record "Sales Header"; Total: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.md b/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.md new file mode 100644 index 0000000..bf563b4 --- /dev/null +++ b/microsoft/knowledge/events/do-not-add-ishandled-to-an-existing-event.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [ishandled, semantic-change, event-contract, backward-compatibility, onbefore, integration-event, subscribers] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not add IsHandled to an existing event + +## Description + +Adding a `var IsHandled: Boolean` parameter to an event that already shipped without one silently changes the event's purpose β€” from a plain notification into an overridable seam. Existing subscribers were written against a "notify" contract they never agreed to make skippable, so their behaviour can quietly become wrong or pointless. The safe move is to leave the existing event untouched and introduce a new `OnBefore…` event carrying `IsHandled` at the point you want to make overridable. Existing subscribers keep working against the original event; new subscribers opt into the override seam through the new one. + +## Best Practice + +Keep the existing event as-is and add a separate `OnBeforeX(…; var IsHandled: Boolean)` before the logic you want to make overridable. Two events with distinct, stable contracts are safer than one event whose meaning and signature were changed under its subscribers. + +See sample: `do-not-add-ishandled-to-an-existing-event.good.al`. + +## Anti Pattern + +Mutating a shipped event β€” for example adding `var IsHandled` to `OnAfterCalculateTotal` β€” to retrofit override behaviour, which overloads the event's meaning and undermines existing subscribers. Detection: an `IsHandled` parameter added to a pre-existing event signature rather than introduced through a new dedicated `OnBefore` publisher. + +See sample: `do-not-add-ishandled-to-an-existing-event.bad.al`. diff --git a/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.bad.al b/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.bad.al new file mode 100644 index 0000000..87ef1e3 --- /dev/null +++ b/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.bad.al @@ -0,0 +1,30 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50296 "Critical Op Bad Sample" +{ + procedure PostInvoice(var SalesHeader: Record "Sales Header") + var + IsHandled: Boolean; + begin + // Anti-pattern: IsHandled wraps the entire posting. A subscriber can set + // IsHandled := true and silently skip ledger-entry creation and the + // status update, leaving imbalanced ledgers and orphaned documents. + IsHandled := false; + OnBeforePostInvoice(SalesHeader, IsHandled); + if IsHandled then + exit; + + CreateCustomerLedgerEntry(SalesHeader); + SalesHeader.Status := SalesHeader.Status::Released; + SalesHeader.Modify(true); + end; + + local procedure CreateCustomerLedgerEntry(var SalesHeader: Record "Sales Header") + begin + // Posts the customer ledger entry (critical; must never be skipped). + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforePostInvoice(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.good.al b/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.good.al new file mode 100644 index 0000000..7a2234b --- /dev/null +++ b/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.good.al @@ -0,0 +1,38 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50295 "Critical Op Good Sample" +{ + procedure PostInvoice(var SalesHeader: Record "Sales Header") + var + DiscountAmount: Decimal; + IsHandled: Boolean; + begin + // IsHandled guards only a safe, side-effect-free calculation. + IsHandled := false; + OnBeforeCalculateInvoiceDiscount(SalesHeader, DiscountAmount, IsHandled); + if not IsHandled then + DiscountAmount := 10; + SalesHeader."Invoice Discount Amount" := DiscountAmount; + + // Critical operations always run; no subscriber can bypass them. + CreateCustomerLedgerEntry(SalesHeader); + SalesHeader.Status := SalesHeader.Status::Released; + SalesHeader.Modify(true); + + OnAfterPostInvoice(SalesHeader); + end; + + local procedure CreateCustomerLedgerEntry(var SalesHeader: Record "Sales Header") + begin + // Posts the customer ledger entry (critical; must never be skipped). + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeCalculateInvoiceDiscount(var SalesHeader: Record "Sales Header"; var DiscountAmount: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterPostInvoice(var SalesHeader: Record "Sales Header") + begin + end; +} diff --git a/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md b/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md new file mode 100644 index 0000000..e6941ce --- /dev/null +++ b/microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [ishandled, critical-operations, posting, data-integrity, ledger, integration-event, safety] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not bypass critical operations with IsHandled + +## Description + +The IsHandled override pattern lets a subscriber skip the guarded code entirely. A critical operation is one that cannot stand as an independent, self-contained unit β€” code whose partial execution or omission leaves the system inconsistent (imbalanced ledgers, orphaned documents, gaps in a number series, or skipped permission checks). That is acceptable around a pure, side-effect-free calculation, but dangerous around critical operations β€” posting, ledger-entry creation, number-series consumption, and referential-integrity or permission validation. Wrapping those in `OnBeforeX(…; var IsHandled); if IsHandled then exit;` lets any subscriber silently suppress them, risking imbalanced ledgers, orphaned documents, skipped permission checks, or duplicated numbers β€” corruption that surfaces far from the subscriber that caused it. Make the calculation overridable, not the commit: expose the value computation through IsHandled, or offer a regular `OnAfter…` event to adjust results, while the critical work runs unconditionally. + +## Best Practice + +Scope IsHandled to a safe value-calculation block and run the critical operations unconditionally afterwards; or expose a positive `OnAfter…` event for subscribers to adjust results, rather than a bypass around the commit. + +See sample: `do-not-bypass-critical-operations-with-ishandled.good.al`. + +## Anti Pattern + +An `OnBefore…` IsHandled guard wrapping a posting or ledger routine β€” `if IsHandled then exit;` around the code that creates ledger entries and updates document status β€” letting subscribers skip the commit. Detection: an `if IsHandled then exit;` whose skipped body performs posting, ledger writes, number-series consumption, or integrity and permission validation. + +See sample: `do-not-bypass-critical-operations-with-ishandled.bad.al`. diff --git a/microsoft/knowledge/events/do-not-publish-events-inside-loops.bad.al b/microsoft/knowledge/events/do-not-publish-events-inside-loops.bad.al new file mode 100644 index 0000000..daaf91c --- /dev/null +++ b/microsoft/knowledge/events/do-not-publish-events-inside-loops.bad.al @@ -0,0 +1,22 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50266 "Loop Event Bad Sample" +{ + procedure ProcessLines(var SalesLine: Record "Sales Line") + begin + if SalesLine.FindSet() then + repeat + // Anti-pattern: an event raised on every iteration. Each + // subscriber runs once per line, so the cost scales with the + // row count and large batches can time out. + OnProcessLine(SalesLine); + + SalesLine."Line Amount" := SalesLine.Quantity * SalesLine."Unit Price"; + SalesLine.Modify(true); + until SalesLine.Next() = 0; + end; + + [IntegrationEvent(false, false)] + local procedure OnProcessLine(var SalesLine: Record "Sales Line") + begin + end; +} diff --git a/microsoft/knowledge/events/do-not-publish-events-inside-loops.good.al b/microsoft/knowledge/events/do-not-publish-events-inside-loops.good.al new file mode 100644 index 0000000..7decb4d --- /dev/null +++ b/microsoft/knowledge/events/do-not-publish-events-inside-loops.good.al @@ -0,0 +1,28 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50265 "Loop Event Good Sample" +{ + procedure ProcessLines(var SalesLine: Record "Sales Line") + begin + // Fire once before the loop; subscribers act on the whole set. + OnBeforeProcessLines(SalesLine); + + if SalesLine.FindSet() then + repeat + SalesLine."Line Amount" := SalesLine.Quantity * SalesLine."Unit Price"; + SalesLine.Modify(true); + until SalesLine.Next() = 0; + + // Fire once after the loop. + OnAfterProcessLines(SalesLine); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeProcessLines(var SalesLine: Record "Sales Line") + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterProcessLines(var SalesLine: Record "Sales Line") + begin + end; +} diff --git a/microsoft/knowledge/events/do-not-publish-events-inside-loops.md b/microsoft/knowledge/events/do-not-publish-events-inside-loops.md new file mode 100644 index 0000000..badbf28 --- /dev/null +++ b/microsoft/knowledge/events/do-not-publish-events-inside-loops.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [performance, loops, event-publishing, batch, onbefore, onafter, subscriber-cost] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not publish events inside loops + +## Description + +Raising an event on every iteration of a loop multiplies the cost of every subscriber by the number of records. A subscriber doing even a little work per call can turn a fast batch into a timeout when the loop runs over thousands of rows, and the publisher has no control over how expensive a subscriber is. Unless a genuine per-row hook is required, publish once before the loop and once after it, passing enough context β€” filters, a key, or a buffer β€” for subscribers to act on the whole set at once. Generated code tends to drop an event inside the `repeat … until` without weighing the per-iteration multiplier. + +## Best Practice + +Raise `OnBeforeProcessLines` before the loop and `OnAfterProcessLines` after it, outside the `repeat … until`, so each subscriber runs once per batch rather than once per row. Give those events the record or filters they need to operate on the whole set. + +See sample: `do-not-publish-events-inside-loops.good.al`. + +## Anti Pattern + +An event raised inside the loop body, fired once per iteration, so subscriber cost scales with the row count and large batches slow down or time out. Detection: an `OnBefore…`/`OnAfter…`/`On…` raise located between `repeat` and `until` in a record loop. + +See sample: `do-not-publish-events-inside-loops.bad.al`. diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al new file mode 100644 index 0000000..94b50f3 --- /dev/null +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al @@ -0,0 +1,31 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50241 "IsHandled Init Bad Sample" +{ + procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") + var + DiscountPct: Decimal; + IsHandled: Boolean; + begin + // IsHandled is never initialized before the first raise, so flow depends + // on the variable's default rather than an explicit, documented intent. + OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); + if not IsHandled then + DiscountPct := 5; + + // Bug: IsHandled is not reset. If the first subscriber set it true, the + // payment-discount default below is silently skipped too. + OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); + if not IsHandled then + DiscountPct += 2; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al new file mode 100644 index 0000000..190e321 --- /dev/null +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al @@ -0,0 +1,31 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50240 "IsHandled Init Good Sample" +{ + procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") + var + DiscountPct: Decimal; + IsHandled: Boolean; + begin + IsHandled := false; + OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); + if not IsHandled then + DiscountPct := 5; + + // Reset before reusing the same variable for the next event so a + // subscriber that handled the first raise can't suppress this one. + IsHandled := false; + OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); + if not IsHandled then + DiscountPct += 2; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md new file mode 100644 index 0000000..acfb54a --- /dev/null +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [ishandled, initialization, deterministic, onbefore, reset, integration-event, control-flow] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Initialize IsHandled to false before publishing + +## Description + +A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable in by reference, so its incoming value decides whether the default logic is skipped. A freshly declared Boolean starts as `false`, but the same variable is frequently reused to raise several events in one routine, and after the first raise it may already be `true`. Assigning `IsHandled := false;` on the line immediately before every raise makes the control flow deterministic and self-documenting, and prevents a stale `true` from silently suppressing logic the author never meant to make skippable. Generated code often reuses one `IsHandled` across several raises without resetting it. + +## Best Practice + +Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later. This applies only to events that carry a `var IsHandled: Boolean`; an `OnBefore` event with no `IsHandled` parameter needs no reset. + +See sample: `initialize-ishandled-to-false-before-publishing.good.al`. + +## Anti Pattern + +Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise that passes an `IsHandled` variable without an intervening `IsHandled := false;`. + +See sample: `initialize-ishandled-to-false-before-publishing.bad.al`. diff --git a/microsoft/knowledge/events/name-event-parameters-without-abbreviations.bad.al b/microsoft/knowledge/events/name-event-parameters-without-abbreviations.bad.al new file mode 100644 index 0000000..038acab --- /dev/null +++ b/microsoft/knowledge/events/name-event-parameters-without-abbreviations.bad.al @@ -0,0 +1,15 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50276 "Param Naming Bad Sample" +{ + procedure RegisterPayment(var SalesHdr: Record "Sales Header"; DocNo: Code[20]; Amt: Decimal) + begin + // Anti-pattern: abbreviated parameter names force every subscriber to + // guess what SalesHdr, DocNo and Amt mean. + OnAfterRegisterPayment(SalesHdr, DocNo, Amt); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterRegisterPayment(var SalesHdr: Record "Sales Header"; DocNo: Code[20]; Amt: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/events/name-event-parameters-without-abbreviations.good.al b/microsoft/knowledge/events/name-event-parameters-without-abbreviations.good.al new file mode 100644 index 0000000..db05c34 --- /dev/null +++ b/microsoft/knowledge/events/name-event-parameters-without-abbreviations.good.al @@ -0,0 +1,14 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50275 "Param Naming Good Sample" +{ + procedure RegisterPayment(var SalesHeader: Record "Sales Header"; DocumentNo: Code[20]; Amount: Decimal) + begin + // Full, spelled-out names make the event contract self-explanatory. + OnAfterRegisterPayment(SalesHeader, DocumentNo, Amount); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterRegisterPayment(var SalesHeader: Record "Sales Header"; DocumentNo: Code[20]; Amount: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/events/name-event-parameters-without-abbreviations.md b/microsoft/knowledge/events/name-event-parameters-without-abbreviations.md new file mode 100644 index 0000000..539dd06 --- /dev/null +++ b/microsoft/knowledge/events/name-event-parameters-without-abbreviations.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [parameter-naming, readability, conventions, event-parameters, no-abbreviations, integration-event, clarity] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Name event parameters without abbreviations + +## Description + +Event parameter names are part of the public contract a subscriber codes against, so they must be self-explanatory. Record parameters take the full table name with the spaces removed β€” `SalesHeader` for `"Sales Header"`, not `SalesHdr` or `SH`. Simple parameters get a descriptive, spelled-out name β€” `DocumentNo`, not `DocNo`; `Amount`, not `Amt`. Abbreviated names force every subscriber author to guess intent and tend to be inconsistent across a codebase, where the same concept appears under several contractions. The cost of a clear name is paid once at the publisher; the cost of a cryptic one is paid by every subscriber that has to decode it. + +## Best Practice + +Use full, unabbreviated names: `(SalesHeader: Record "Sales Header"; DocumentNo: Code[20]; Amount: Decimal)`. Record parameters mirror the table name without spaces, and value parameters read as whole words so the contract is unambiguous. + +See sample: `name-event-parameters-without-abbreviations.good.al`. + +## Anti Pattern + +Abbreviated parameter names (`SalesHdr`, `DocNo`, `Amt`) that obscure meaning and vary across publishers, so subscribers must guess what each one holds. Detection: event parameters whose names are truncated forms of the table name or contracted words rather than the full term. + +See sample: `name-event-parameters-without-abbreviations.bad.al`. diff --git a/microsoft/knowledge/events/name-events-by-publisher-position.bad.al b/microsoft/knowledge/events/name-events-by-publisher-position.bad.al new file mode 100644 index 0000000..f4799cb --- /dev/null +++ b/microsoft/knowledge/events/name-events-by-publisher-position.bad.al @@ -0,0 +1,35 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50256 "Event Naming Bad Sample" +{ + procedure PostSalesLine(var SalesLine: Record "Sales Line") + var + LineAmount: Decimal; + begin + // Anti-pattern: names don't encode the host routine or the + // before/after position, so subscribers can't tell when they fire. + BeforePost(SalesLine); + + LineAmount := SalesLine.Quantity * SalesLine."Unit Price"; + MyCustomSalesEvent(SalesLine, LineAmount); + + SalesLine."Line Amount" := LineAmount; + SalesLine.Modify(true); + + SalesLineEvent(SalesLine); + end; + + [IntegrationEvent(false, false)] + local procedure BeforePost(var SalesLine: Record "Sales Line") + begin + end; + + [IntegrationEvent(false, false)] + local procedure MyCustomSalesEvent(var SalesLine: Record "Sales Line"; var LineAmount: Decimal) + begin + end; + + [IntegrationEvent(false, false)] + local procedure SalesLineEvent(var SalesLine: Record "Sales Line") + begin + end; +} diff --git a/microsoft/knowledge/events/name-events-by-publisher-position.good.al b/microsoft/knowledge/events/name-events-by-publisher-position.good.al new file mode 100644 index 0000000..c2c5374 --- /dev/null +++ b/microsoft/knowledge/events/name-events-by-publisher-position.good.al @@ -0,0 +1,64 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50255 "Event Naming Good Sample" +{ + procedure PostSalesLine(var SalesLine: Record "Sales Line") + var + LineAmount: Decimal; + begin + // Start of the routine: OnBefore. + OnBeforePostSalesLine(SalesLine); + + LineAmount := SalesLine.Quantity * SalesLine."Unit Price"; + // Middle of the routine: OnOnAfter. + OnPostSalesLineOnAfterCalcAmounts(SalesLine, LineAmount); + + SalesLine."Line Amount" := LineAmount; + SalesLine.Modify(true); + + // End of the routine: OnAfter. + OnAfterPostSalesLine(SalesLine); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforePostSalesLine(var SalesLine: Record "Sales Line") + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnPostSalesLineOnAfterCalcAmounts(var SalesLine: Record "Sales Line"; var LineAmount: Decimal) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterPostSalesLine(var SalesLine: Record "Sales Line") + begin + end; + + // Same position-naming convention applies to events raised from table and + // report triggers, not just codeunit procedures. + + // Raised at the end of a table field's OnValidate trigger (for example + // Customer."No." OnValidate): the position is "after", so OnAfter. + procedure HandleCustomerNoValidated(var Customer: Record Customer) + begin + OnAfterValidateCustomerNo(Customer); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterValidateCustomerNo(var Customer: Record Customer) + begin + end; + + // Raised before a report prints a line from its processing trigger (for + // example a dataitem OnAfterGetRecord): the position is "before", so + // OnBefore. + procedure HandleReportLineProcessing(var SalesLine: Record "Sales Line") + begin + OnBeforeReportPrintLine(SalesLine); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeReportPrintLine(var SalesLine: Record "Sales Line") + begin + end; +} diff --git a/microsoft/knowledge/events/name-events-by-publisher-position.md b/microsoft/knowledge/events/name-events-by-publisher-position.md new file mode 100644 index 0000000..cd8ac65 --- /dev/null +++ b/microsoft/knowledge/events/name-events-by-publisher-position.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [event-naming, onbefore, onafter, conventions, discoverability, integration-event, publisher] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Name events by publisher position + +## Description + +An event name should tell a subscriber where in the publisher the event fires. The convention encodes the position: an event at the very start of a procedure or trigger is `OnBefore`; one at the very end is `OnAfter`; one in the middle names both the host routine and the local boundary, as `OnOnBefore` or `OnOnAfter`. Consistent, position-encoding names make events discoverable and predictable, and let developers and tooling reason about firing order without reading the publisher. Ad-hoc names such as `MyCustomEvent` or `BeforePost` hide where the event fires and break the conventions the ecosystem relies on. + +## Best Practice + +Name by position: `OnBeforePostSalesLine` and `OnAfterPostSalesLine` at the routine boundaries, and `OnPostSalesLineOnAfterCalcAmounts` for an event raised partway through `PostSalesLine` after an amount calculation. The name alone then tells a subscriber both the host routine and the exact point it runs. + +See sample: `name-events-by-publisher-position.good.al`. + +## Anti Pattern + +Ad-hoc event names that omit the host routine or the before/after position (`MyCustomSalesEvent`, `BeforePost`, `SalesLineEvent`), leaving subscribers unable to tell when the event fires relative to the publisher's logic. Detection: publisher names that do not follow the `OnBefore`/`OnAfter` or `OnOnBefore`/`OnAfter` patterns. + +See sample: `name-events-by-publisher-position.bad.al`. diff --git a/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.bad.al b/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.bad.al new file mode 100644 index 0000000..7fc3e8d --- /dev/null +++ b/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.bad.al @@ -0,0 +1,27 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50261 "Reuse Event Bad Sample" +{ + procedure ProcessOrder(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20]) + var + IsHandled: Boolean; + begin + IsHandled := false; + // Anti-pattern: a near-duplicate event raised right next to the original, + // differing only by an extra parameter β€” two consecutive events where a + // single extended event would do. + OnBeforeProcessOrder(SalesHeader, IsHandled); + OnBeforeProcessOrderWithCustomer(SalesHeader, CustomerNo, IsHandled); + if IsHandled then + exit; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeProcessOrder(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeProcessOrderWithCustomer(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20]; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.good.al b/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.good.al new file mode 100644 index 0000000..0d64906 --- /dev/null +++ b/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.good.al @@ -0,0 +1,20 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50260 "Reuse Event Good Sample" +{ + procedure ProcessOrder(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20]) + var + IsHandled: Boolean; + begin + IsHandled := false; + // A single event, extended with CustomerNo appended at the end, covers + // the need; no second event is raised beside it. + OnBeforeProcessOrder(SalesHeader, CustomerNo, IsHandled); + if IsHandled then + exit; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeProcessOrder(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20]; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.md b/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.md new file mode 100644 index 0000000..3136023 --- /dev/null +++ b/microsoft/knowledge/events/prefer-reusing-or-extending-existing-events.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [event-reuse, duplication, consecutive-events, extension-point, onbefore, integration-event, maintainability] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Prefer reusing or extending existing events + +## Description + +Before adding a publisher, check whether an event already fires at that point in the code. Two related smells signal that you should reuse or extend instead of adding one. The first is a brand-new event placed directly next to an existing one β€” two consecutive event raises with no logic between them, which gives subscribers two seams where one belongs. The second is a near-duplicate event that differs from an existing one only by an extra parameter. Both bloat the publisher surface and leave subscribers unsure which event to pick. Prefer subscribing to the existing event, or extending it by appending the parameter you need, over introducing a parallel one. + +## Best Practice + +When the data you need is already exposed at an existing event, subscribe to it. When the event lacks a parameter, extend that event by appending the parameter at the end β€” one publisher, one raise β€” rather than adding a second event beside it. + +See sample: `prefer-reusing-or-extending-existing-events.good.al`. + +## Anti Pattern + +Adding a second event raise immediately after an existing one, or creating `OnBeforeProcessOrderWithCustomer` next to `OnBeforeProcessOrder` just to add a single parameter. Detection: two consecutive `OnBefore…`/`OnAfter…` raises with no logic between them, or near-duplicate event names differing only by a parameter-describing suffix. + +See sample: `prefer-reusing-or-extending-existing-events.bad.al`. diff --git a/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.bad.al b/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.bad.al new file mode 100644 index 0000000..f08e930 --- /dev/null +++ b/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.bad.al @@ -0,0 +1,15 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50281 "Sender This Bad Sample" +{ + procedure ProcessOrder(OrderNo: Code[20]) + begin + OnBeforeProcessOrder(OrderNo); + end; + + // Anti-pattern: IncludeSender = true is used only to expose the publisher + // instance to subscribers; a codeunit can pass 'this' explicitly instead. + [IntegrationEvent(true, false)] + local procedure OnBeforeProcessOrder(OrderNo: Code[20]) + begin + end; +} diff --git a/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.good.al b/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.good.al new file mode 100644 index 0000000..adedb6a --- /dev/null +++ b/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.good.al @@ -0,0 +1,14 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50280 "Sender This Good Sample" +{ + procedure ProcessOrder(OrderNo: Code[20]) + begin + // Pass the current instance explicitly as a typed Sender parameter. + OnBeforeProcessOrder(OrderNo, this); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeProcessOrder(OrderNo: Code[20]; Sender: Codeunit "Sender This Good Sample") + begin + end; +} diff --git a/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.md b/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.md new file mode 100644 index 0000000..c32bd0d --- /dev/null +++ b/microsoft/knowledge/events/prefer-this-over-includesender-in-codeunit-events.md @@ -0,0 +1,26 @@ +--- +bc-version: [25..] +domain: events +keywords: [this-keyword, includesender, sender, codeunit, self-reference, integration-event, type-safety] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Prefer this over IncludeSender in codeunit events + +## Description + +Some publishers set `IncludeSender` to `true` on `[IntegrationEvent]` or `[BusinessEvent]` so subscribers receive the publishing object as an implicit sender parameter. From Business Central 2024 release wave 2, a codeunit can instead pass itself explicitly with the `this` keyword as a normal, strongly-typed `Sender` parameter. Explicit passing is clearer at both the publisher and the subscriber: the sender appears in the signature, it is concretely typed to the publishing codeunit, and it avoids the implicit-parameter mechanics of `IncludeSender`. Reserve `IncludeSender = true` for cases where the sender genuinely cannot be passed explicitly. This guidance applies to code targeting Business Central 2024 release wave 2 or later, where the `this` keyword is available. + +## Best Practice + +Declare the publisher `[IntegrationEvent(false, false)]` with an explicit `Sender: Codeunit "…"` parameter and raise it with `this`, for example `OnBeforeProcessOrder(OrderNo, this);`. Subscribers then receive a typed sender they can call directly. + +See sample: `prefer-this-over-includesender-in-codeunit-events.good.al`. + +## Anti Pattern + +Relying on `[IntegrationEvent(true, …)]` solely to hand subscribers the publisher instance, where a codeunit could pass `this` explicitly as a typed parameter. Detection: `IncludeSender = true` on a codeunit event whose only purpose is to expose the sender, in code targeting Business Central 2024 release wave 2 or later. + +See sample: `prefer-this-over-includesender-in-codeunit-events.bad.al`. diff --git a/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.bad.al b/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.bad.al new file mode 100644 index 0000000..f4d315f --- /dev/null +++ b/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.bad.al @@ -0,0 +1,16 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50271 "Temp Param Bad Sample" +{ + procedure SummarizeLines(var SalesLineBuffer: Record "Sales Line" temporary) + begin + // Anti-pattern: the parameter is temporary but isn't named with a Temp + // prefix, so subscribers can't tell the data isn't persisted and may + // rely on writes that are discarded. + OnAfterSummarizeLines(SalesLineBuffer); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterSummarizeLines(var SalesLineBuffer: Record "Sales Line" temporary) + begin + end; +} diff --git a/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.good.al b/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.good.al new file mode 100644 index 0000000..b82c971 --- /dev/null +++ b/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.good.al @@ -0,0 +1,14 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50270 "Temp Param Good Sample" +{ + procedure SummarizeLines(var TempSalesLineBuffer: Record "Sales Line" temporary) + begin + // The Temp prefix tells subscribers the buffer isn't persisted. + OnAfterSummarizeLines(TempSalesLineBuffer); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterSummarizeLines(var TempSalesLineBuffer: Record "Sales Line" temporary) + begin + end; +} diff --git a/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.md b/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.md new file mode 100644 index 0000000..0a952a5 --- /dev/null +++ b/microsoft/knowledge/events/prefix-temporary-record-event-parameters-with-temp.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [temporary-record, naming, event-parameters, buffer, temp-prefix, integration-event, conventions] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Prefix temporary record event parameters with Temp + +## Description + +When a record passed to an event is a temporary record β€” an in-memory buffer not persisted to the database β€” its parameter name must start with `Temp`. The prefix is the only reliable signal a subscriber has that writes to the record will not reach the database and that the data is scoped to the current call. Without it, subscribers may treat buffer data as persisted: calling `Modify` or `Insert` expecting durability, or reading it as the authoritative table, which leads to silent data loss and confusing behaviour. The `temporary` keyword sits on the variable declaration and is not visible at the subscriber, so the name has to carry the meaning. + +## Best Practice + +Name temporary record parameters with a `Temp` prefix, for example `var TempSalesLineBuffer: Record "Sales Line" temporary`, so every subscriber sees immediately that the record is an in-memory buffer and treats writes accordingly. + +See sample: `prefix-temporary-record-event-parameters-with-temp.good.al`. + +## Anti Pattern + +A temporary record parameter named without the `Temp` prefix (`var SalesLineBuffer: Record "Sales Line" temporary`), so subscribers cannot tell the record is non-persistent and may rely on writes that are silently discarded. Detection: an event parameter declared `temporary` whose name does not start with `Temp`. + +See sample: `prefix-temporary-record-event-parameters-with-temp.bad.al`. diff --git a/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.bad.al b/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.bad.al new file mode 100644 index 0000000..1151e26 --- /dev/null +++ b/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.bad.al @@ -0,0 +1,32 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50246 "OnAfter Preserve Bad Sample" +{ + procedure ReleaseDocument(var SalesHeader: Record "Sales Header") + var + IsHandled: Boolean; + begin + IsHandled := false; + OnBeforeReleaseDocument(SalesHeader, IsHandled); + + // Bug: returning here also skips OnAfterReleaseDocument below, so + // subscribers that rely on the after-event stop running whenever + // another extension handles the OnBefore. + if IsHandled then + exit; + + SalesHeader.Status := SalesHeader.Status::Released; + SalesHeader.Modify(true); + + OnAfterReleaseDocument(SalesHeader); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeReleaseDocument(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterReleaseDocument(var SalesHeader: Record "Sales Header") + begin + end; +} diff --git a/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.good.al b/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.good.al new file mode 100644 index 0000000..4740877 --- /dev/null +++ b/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.good.al @@ -0,0 +1,30 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50245 "OnAfter Preserve Good Sample" +{ + procedure ReleaseDocument(var SalesHeader: Record "Sales Header") + var + IsHandled: Boolean; + begin + IsHandled := false; + OnBeforeReleaseDocument(SalesHeader, IsHandled); + + // Skip only the default body, not the routine, so OnAfter still fires. + if not IsHandled then begin + SalesHeader.Status := SalesHeader.Status::Released; + SalesHeader.Modify(true); + end; + + // Fires whether or not a subscriber handled the body above. + OnAfterReleaseDocument(SalesHeader); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeReleaseDocument(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterReleaseDocument(var SalesHeader: Record "Sales Header") + begin + end; +} diff --git a/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.md b/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.md new file mode 100644 index 0000000..4ff958c --- /dev/null +++ b/microsoft/knowledge/events/preserve-onafter-execution-when-ishandled-skips-the-body.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [ishandled, onafter, event-pairing, control-flow, guard, integration-event, side-effects] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Preserve OnAfter execution when IsHandled skips the body + +## Description + +A routine that exposes both an `OnBefore…` event (with `var IsHandled`) and a paired `OnAfter…` event has a subtle trap. The common `if IsHandled then exit;` guard returns from the whole routine, so when a subscriber handles the OnBefore the OnAfter event never fires. Subscribers that depend on OnAfter β€” logging, downstream integration, dependent updates β€” then silently stop running whenever some other extension overrides the body. The fix is to skip only the default body, not the routine, so the OnAfter still publishes. The two seams are independent: overriding the work should not cancel the notification that the work happened. + +## Best Practice + +Wrap only the default work in `if not IsHandled then begin … end;` and keep the `OnAfterX(…)` raise after that block, outside the guard, so it always fires regardless of whether a subscriber handled the OnBefore. This keeps the override seam and the after-notification independent, which is what subscribers expect. + +See sample: `preserve-onafter-execution-when-ishandled-skips-the-body.good.al`. + +## Anti Pattern + +Guarding with `if IsHandled then exit;` and placing the `OnAfterX` raise later in the same routine, so handling the OnBefore short-circuits the whole procedure and the OnAfter event is skipped along with the body. Detection: an `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event after that point. + +See sample: `preserve-onafter-execution-when-ishandled-skips-the-body.bad.al`. diff --git a/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.bad.al b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.bad.al new file mode 100644 index 0000000..5942ebd --- /dev/null +++ b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.bad.al @@ -0,0 +1,36 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +table 50226 "Reservation Entry Bad Sample" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + field(4; Reserved; Boolean) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } +} + +codeunit 50227 "Reservation Post Bad Sample" +{ + procedure Reserve(var ReservationEntry: Record "Reservation Entry Bad Sample") + begin + // Anti-pattern: the operation exposes no OnBefore/OnAfter seam, and the + // logic that should be the routine's own work lives in the event body + // below instead. Partners must overwrite this routine to change it. + OnReserve(ReservationEntry); + end; + + // Anti-pattern: business logic inside an integration-event publisher. A + // publisher must be a thin, empty hook; logic placed here runs on every + // raise and cannot be overridden, which defeats the event entirely. + [IntegrationEvent(false, false)] + local procedure OnReserve(var ReservationEntry: Record "Reservation Entry Bad Sample") + begin + ReservationEntry.Reserved := true; + ReservationEntry.Modify(true); + end; +} diff --git a/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.good.al b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.good.al new file mode 100644 index 0000000..ef67a3e --- /dev/null +++ b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.good.al @@ -0,0 +1,43 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +table 50224 "Reservation Entry Sample" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + field(4; Reserved; Boolean) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } +} + +codeunit 50225 "Reservation Post Good Sample" +{ + procedure Reserve(var ReservationEntry: Record "Reservation Entry Sample") + var + IsHandled: Boolean; + begin + OnBeforeReserve(ReservationEntry, IsHandled); + if IsHandled then + exit; + + ReservationEntry.Reserved := true; + ReservationEntry.Modify(true); + + OnAfterReserve(ReservationEntry); + end; + + // Thin publishers: empty bodies, the calling routine owns the logic. + [IntegrationEvent(false, false)] + local procedure OnBeforeReserve(var ReservationEntry: Record "Reservation Entry Sample"; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterReserve(var ReservationEntry: Record "Reservation Entry Sample") + begin + end; +} diff --git a/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md new file mode 100644 index 0000000..30c94df --- /dev/null +++ b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [integration-event, onbefore, onafter, extension-point, thin-publisher, publisher-body, extensibility] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Publish thin OnBefore/OnAfter integration events to expose extension points + +## Description + +A key operation β€” a posting, release, or validation routine β€” becomes a hard wall for partners when it ships no integration events: the only way to change it is to overwrite or duplicate the base code. The Business Central remedy is to raise thin `OnBeforeX`/`OnAfterX` integration events at the operation's boundaries, passing `var Rec` and the relevant parameters so subscribers have what they need. An equally common defect is the inverse: putting business logic *inside* the publisher method body. An event publisher is a hook, not a procedure β€” its body must be empty, and the platform even forbids variables, return values, and code other than comments in it. LLMs both omit the extension points and, when they do add an event, wrongly fill its body with logic. + +## Best Practice + +Wrap the operation's core with events: raise `OnBeforeX(var Rec, var IsHandled)` before the default work and `OnAfterX(var Rec)` once it succeeds, at the natural boundaries of the routine. Declare each publisher `[IntegrationEvent(false, false)] local procedure` with an empty body and let the calling routine β€” never the publisher β€” own the logic. Pass records by `var` so subscribers can read and adjust them, and include the parameters a subscriber would need to act. This gives partners a stable seam without touching base code. + +See sample: `publish-thin-onbefore-onafter-integration-events.good.al`. + +## Anti Pattern + +Business logic placed inside an `[IntegrationEvent]` publisher method, so the "event" actually mutates state every time it is raised β€” defeating the hook and surprising every reader β€” or a core operation that exposes no extension points at all, forcing partners to overwrite or duplicate it. Detection: an `[IntegrationEvent]`/`[BusinessEvent]` method whose body contains statements rather than being empty, or a posting/validation routine with no surrounding `OnBefore`/`OnAfter` publishers. + +See sample: `publish-thin-onbefore-onafter-integration-events.bad.al`. diff --git a/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.bad.al b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.bad.al new file mode 100644 index 0000000..766a30a --- /dev/null +++ b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.bad.al @@ -0,0 +1,38 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. + +// Anti-pattern 1: no OnBefore/IsHandled hook. A partner cannot replace this +// rule without overwriting base code, so the behaviour is not extensible. +codeunit 50222 "Shipping Charge NoHook Bad" +{ + procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal + begin + if OrderAmount >= 1000 then + Charge := 0 + else + Charge := 49; + end; +} + +// Anti-pattern 2: the hook exists but the 'if IsHandled then exit;' guard is +// missing, so the default logic still runs after a subscriber handled the call. +codeunit 50223 "Shipping Charge Guard Bad" +{ + procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal + var + IsHandled: Boolean; + begin + OnBeforeCalculateShippingCharge(OrderAmount, Charge, IsHandled); + + // Bug: no 'if IsHandled then exit;' here. Even when a subscriber set + // Charge and IsHandled := true, the default below overwrites the result. + if OrderAmount >= 1000 then + Charge := 0 + else + Charge := 49; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeCalculateShippingCharge(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.good.al b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.good.al new file mode 100644 index 0000000..6b47535 --- /dev/null +++ b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.good.al @@ -0,0 +1,37 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50220 "Shipping Charge Good Sample" +{ + procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal + var + IsHandled: Boolean; + begin + // Give extensions a sanctioned seam to replace the calculation, then + // skip the default logic when a subscriber has handled it. + OnBeforeCalculateShippingCharge(OrderAmount, Charge, IsHandled); + if IsHandled then + exit(Charge); + + if OrderAmount >= 1000 then + Charge := 0 + else + Charge := 49; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeCalculateShippingCharge(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean) + begin + end; +} + +codeunit 50221 "Shipping Charge Sub Good Sample" +{ + // A partner replaces the flat rate with a contract-specific rule. + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Shipping Charge Good Sample", 'OnBeforeCalculateShippingCharge', '', false, false)] + local procedure ApplyContractRate(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean) + begin + if IsHandled then + exit; + Charge := OrderAmount * 0.02; + IsHandled := true; + end; +} diff --git a/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md new file mode 100644 index 0000000..d273589 --- /dev/null +++ b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [ishandled, overridable, onbefore, integration-event, extensibility, event-override, subscriber-hook] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use the IsHandled pattern to make base behaviour overridable + +## Description + +AL has no method overriding, so a `procedure` that runs its body unconditionally cannot be replaced by an extension without editing base code. The established Business Central seam for substituting default behaviour is the `IsHandled` pattern: the routine raises an `OnBefore…` integration event carrying a `var IsHandled: Boolean`, then exits early when a subscriber has set it. This hands a partner a sanctioned hook to replace the logic instead of overwriting the routine. LLMs trained on languages with inheritance emit routines whose logic always runs and expose no `OnBefore`/`IsHandled` seam, so the behaviour silently cannot be overridden. + +## Best Practice + +Raise `OnBeforeX(…, IsHandled)` as the first step of the routine and guard with `if IsHandled then exit;` before any default logic runs. Declare the publisher `[IntegrationEvent(false, false)] local procedure OnBeforeX(…; var IsHandled: Boolean)` with an empty body, and keep `IsHandled` a `var` parameter so a subscriber can write to it. A subscriber that replaces the behaviour does its work and sets `IsHandled := true`; one that only augments leaves it untouched and guards with `if IsHandled then exit;` itself. Reserve the override hook for cases where a partner genuinely needs to replace logic β€” when the goal is only to react, a positive `OnAfter` event is the better seam. + +See sample: `use-ishandled-to-make-base-behaviour-overridable.good.al`. + +## Anti Pattern + +Two shapes. First, a routine whose default logic always runs because there is no `OnBefore…`/`IsHandled` hook at all β€” extensions cannot change it without overwriting base code. Second, a routine that raises `OnBeforeX(IsHandled)` but omits the `if IsHandled then exit;` guard, so the default logic still executes after a subscriber set `IsHandled := true`, duplicating work and side effects. Detection: an `OnBefore` publisher with a `var IsHandled: Boolean` parameter whose caller never tests `IsHandled`, or a public routine doing non-trivial work with no overridable seam. + +See sample: `use-ishandled-to-make-base-behaviour-overridable.bad.al`. diff --git a/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.bad.al b/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.bad.al new file mode 100644 index 0000000..6c3a332 --- /dev/null +++ b/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.bad.al @@ -0,0 +1,22 @@ +codeunit 50217 "Standard Discount Calc Bad" +{ + procedure CalculateDiscount(Amount: Decimal): Decimal + begin + if Amount > 1000 then + exit(Amount * 0.1); + exit(0); + end; +} + +codeunit 50216 "Order Total Bad" +{ + // Anti-pattern: the dependency is a concrete codeunit type, so a test + // cannot substitute a double - it always runs the production rule. + var + DiscountCalc: Codeunit "Standard Discount Calc Bad"; + + procedure NetAmount(Amount: Decimal): Decimal + begin + exit(Amount - DiscountCalc.CalculateDiscount(Amount)); + end; +} diff --git a/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.good.al b/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.good.al new file mode 100644 index 0000000..e709d22 --- /dev/null +++ b/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.good.al @@ -0,0 +1,51 @@ +interface IDiscountCalculation +{ + procedure CalculateDiscount(Amount: Decimal): Decimal; +} + +codeunit 50213 "Standard Discount Calc" implements IDiscountCalculation +{ + procedure CalculateDiscount(Amount: Decimal): Decimal + begin + // Production rule: 10% off amounts over 1000. + if Amount > 1000 then + exit(Amount * 0.1); + exit(0); + end; +} + +codeunit 50214 "Test Discount Calc" implements IDiscountCalculation +{ + // Lightweight test double: a fixed, predictable value so a test can assert + // order totals without depending on the production discount rule. + procedure CalculateDiscount(Amount: Decimal): Decimal + begin + exit(100); + end; +} + +codeunit 50215 "Order Total" +{ + var + DiscountCalc: Interface IDiscountCalculation; + + // Production wiring: a codeunit assigns directly to the interface variable. + procedure UseProductionCalculation() + var + StdCalc: Codeunit "Standard Discount Calc"; + begin + DiscountCalc := StdCalc; + end; + + // Setter injection: a test passes "Test Discount Calc" instead, with no + // enum and no change to the consumer. The dependency is an interface. + procedure SetDiscountCalculation(NewDiscountCalc: Interface IDiscountCalculation) + begin + DiscountCalc := NewDiscountCalc; + end; + + procedure NetAmount(Amount: Decimal): Decimal + begin + exit(Amount - DiscountCalc.CalculateDiscount(Amount)); + end; +} diff --git a/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.md b/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.md new file mode 100644 index 0000000..e0d50d7 --- /dev/null +++ b/microsoft/knowledge/interfaces/assign-codeunit-to-interface-for-testability.md @@ -0,0 +1,26 @@ +--- +bc-version: [16..] +domain: interfaces +keywords: [interface, dependency-injection, testability, test-double, codeunit, polymorphism, mocking] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Assign a codeunit to an interface variable for injectable, testable dependencies + +## Description + +An interface variable can hold any codeunit that `implements` the interface, assigned directly β€” no enum is required. That is the lever for dependency injection in AL: a consumer depends on the interface, production code injects the real codeunit, and a test injects a lightweight double that returns predictable values. A consumer that instead `var`-declares a concrete `Codeunit` type hardwires the dependency, so a test is forced to exercise the real logic β€” external calls, posting, and all. Interfaces arrived in Business Central 2020 release wave 1; LLMs still default to concrete codeunit variables and miss the seam that makes code testable. + +## Best Practice + +Declare the dependency as an `Interface` variable on the consumer and supply the implementation from outside β€” typically setter injection through a procedure that takes an `Interface` parameter, or a parameter on the entry method. Production passes the real implementation codeunit; a test passes a test-double codeunit that implements the same interface with deterministic behaviour. Because a codeunit assigns to an interface variable directly, no enum or factory is needed for the injectable case. The consumer's logic is then verifiable in isolation. + +See sample: `assign-codeunit-to-interface-for-testability.good.al`. + +## Anti Pattern + +A consumer that declares its dependency as a concrete `Codeunit "..."` variable and calls it directly. The collaborator cannot be substituted, so a unit test either runs the production side effects or cannot cover the consumer at all. Detection signal: a `var` of type `Codeunit ""` used for a collaborator that has β€” or could have β€” an interface, especially one that performs I/O, posting, or external calls. Extract an interface, depend on the interface variable, and inject the implementation. + +See sample: `assign-codeunit-to-interface-for-testability.bad.al`. diff --git a/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.bad.al b/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.bad.al new file mode 100644 index 0000000..81d7727 --- /dev/null +++ b/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.bad.al @@ -0,0 +1,32 @@ +enum 50204 "Shipping Method Bad" +{ + Extensible = true; + + value(0; Standard) { } + value(1; Express) { } +} + +codeunit 50205 "Shipping Charge Bad" +{ + // Anti-pattern: every call site must 'case' over the enum, and every new + // shipping method forces a synchronized edit to each of these blocks. + procedure GetRate(Method: Enum "Shipping Method Bad"; Weight: Decimal): Decimal + begin + case Method of + Method::Standard: + exit(Weight * 1.5); + Method::Express: + exit((Weight * 1.5) + 25); + end; + end; + + procedure GetDeliveryDays(Method: Enum "Shipping Method Bad"): Integer + begin + case Method of + Method::Standard: + exit(5); + Method::Express: + exit(1); + end; + end; +} diff --git a/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.good.al b/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.good.al new file mode 100644 index 0000000..48d2993 --- /dev/null +++ b/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.good.al @@ -0,0 +1,47 @@ +interface IShippingRate +{ + procedure CalculateRate(Weight: Decimal): Decimal; +} + +codeunit 50200 "Standard Shipping Rate" implements IShippingRate +{ + procedure CalculateRate(Weight: Decimal): Decimal + begin + exit(Weight * 1.5); + end; +} + +codeunit 50201 "Express Shipping Rate" implements IShippingRate +{ + procedure CalculateRate(Weight: Decimal): Decimal + begin + exit((Weight * 1.5) + 25); + end; +} + +enum 50202 "Shipping Method" implements IShippingRate +{ + Extensible = true; + + value(0; Standard) + { + Implementation = IShippingRate = "Standard Shipping Rate"; + } + value(1; Express) + { + Implementation = IShippingRate = "Express Shipping Rate"; + } +} + +codeunit 50203 "Shipping Charge" +{ + // Dispatch is automatic: assign the enum to the interface variable and call. + // A new method = one new enum value + one impl codeunit, with no edit here. + procedure GetRate(Method: Enum "Shipping Method"; Weight: Decimal): Decimal + var + RateProvider: Interface IShippingRate; + begin + RateProvider := Method; + exit(RateProvider.CalculateRate(Weight)); + end; +} diff --git a/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md b/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md new file mode 100644 index 0000000..337e0dc --- /dev/null +++ b/microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md @@ -0,0 +1,26 @@ +--- +bc-version: [16..] +domain: interfaces +keywords: [interface, enum-implements-interface, polymorphism, implementation-property, case-statement, variant-behavior, dispatch] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Prefer an interface with enum-backed implementation over a case statement for variant behaviour + +## Description + +When behaviour varies by a discrete "type" β€” a shipping method, a posting strategy, a payment provider β€” the obvious first draft is a `case` over an enum with one branch per variant. That branch logic gets copied to every call site, and every new variant means editing all of them. AL interfaces (Business Central 2020 release wave 1) combined with enum-with-implementation replace that with automatic dispatch: an `interface` declares the contract, an `enum` that `implements` it maps each value to a codeunit, and the consumer assigns the enum value to an interface variable and calls the method. Adding a variant becomes a new enum value plus a new implementation codeunit β€” zero consumer edits. LLMs trained on older AL reach for the `case` block by default and rarely model a variant set as an interface. + +## Best Practice + +Declare an `interface` with the method signatures only (no bodies). Define an `enum` that `implements` the interface and set `Implementation = = ;` on each value, pointing at a codeunit that `implements` the same interface. In the consumer, declare a variable of the interface type, assign the enum value to it, and call the method β€” the platform dispatches to the codeunit mapped to that value. New variants plug in by adding an enum value and its implementation; existing call sites are untouched. The open/closed boundary lives at the enum, not scattered across `case` blocks. + +See sample: `prefer-interface-over-case-branching.good.al`. + +## Anti Pattern + +A `case "Shipping Method" of` block that selects behaviour inline, duplicated across the call sites that need it. Each new method forces a synchronized edit to every block, and a missed branch is a silent gap. Detection signal: a `case` statement over an enum value whose branches choose between variant computations or strategies, especially when the same shape appears in more than one procedure. Replace the enum with one that `implements` an interface, move each branch body into an implementation codeunit, and let dispatch happen through an interface variable. + +See sample: `prefer-interface-over-case-branching.bad.al`. diff --git a/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.bad.al b/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.bad.al new file mode 100644 index 0000000..1372235 --- /dev/null +++ b/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.bad.al @@ -0,0 +1,39 @@ +interface INotifier +{ + procedure Send(Recipient: Text; Body: Text): Boolean; +} + +codeunit 50210 "Email Notifier Bad" implements INotifier +{ + procedure Send(Recipient: Text; Body: Text): Boolean + begin + exit(Recipient <> ''); + end; +} + +enum 50211 "Notification Channel Bad" implements INotifier +{ + Extensible = true; + // No DefaultImplementation declared. + + value(0; Email) + { + Implementation = INotifier = "Email Notifier Bad"; + } + value(1; None) + { + // No Implementation here and no enum-level DefaultImplementation: + // resolving this value to INotifier and calling Send fails at runtime. + } +} + +codeunit 50212 "Notification Dispatch Bad" +{ + procedure Notify(Channel: Enum "Notification Channel Bad"; Recipient: Text; Body: Text): Boolean + var + Notifier: Interface INotifier; + begin + Notifier := Channel; // Channel::None has no implementation + exit(Notifier.Send(Recipient, Body)); // runtime failure for the None value + end; +} diff --git a/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.good.al b/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.good.al new file mode 100644 index 0000000..a3217d5 --- /dev/null +++ b/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.good.al @@ -0,0 +1,49 @@ +interface INotifier +{ + procedure Send(Recipient: Text; Body: Text): Boolean; +} + +codeunit 50206 "Email Notifier" implements INotifier +{ + procedure Send(Recipient: Text; Body: Text): Boolean + begin + // A real implementation would hand the message to an email service. + exit(Recipient <> ''); + end; +} + +codeunit 50207 "Default Notifier" implements INotifier +{ + procedure Send(Recipient: Text; Body: Text): Boolean + begin + // Safe fallback so an unmapped or future channel still resolves to a + // usable object instead of failing where the interface is called. + exit(false); + end; +} + +enum 50208 "Notification Channel" implements INotifier +{ + Extensible = true; + DefaultImplementation = INotifier = "Default Notifier"; + + value(0; Email) + { + Implementation = INotifier = "Email Notifier"; + } + value(1; None) + { + // No explicit Implementation: resolves to DefaultImplementation above. + } +} + +codeunit 50209 "Notification Dispatch" +{ + procedure Notify(Channel: Enum "Notification Channel"; Recipient: Text; Body: Text): Boolean + var + Notifier: Interface INotifier; + begin + Notifier := Channel; + exit(Notifier.Send(Recipient, Body)); + end; +} diff --git a/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md b/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md new file mode 100644 index 0000000..92ef3cf --- /dev/null +++ b/microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md @@ -0,0 +1,26 @@ +--- +bc-version: [16..] +domain: interfaces +keywords: [interface, defaultimplementation, enum-implements-interface, fallback, extensible-enum, implementation-property] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Set DefaultImplementation on an enum so an unmapped value still resolves to an interface + +## Description + +An `enum` that `implements` an interface maps each value to a codeunit through the `Implementation` property. But an extensible enum can carry values that set no `Implementation` β€” values added later by an extension, or a value left intentionally blank. Assigning such a value to an interface variable and calling a method on it fails at runtime unless the enum provides a fallback. The enum-level `DefaultImplementation` property names the codeunit used whenever a value has no explicit `Implementation`, so resolution always yields a usable object. LLMs are generally unaware this property exists and leave the gap open. + +## Best Practice + +On any extensible enum that implements an interface, set `DefaultImplementation = = ;` at the enum level, pointing at a safe implementation that does nothing harmful. Values with their own `Implementation` keep using it; every other value β€” including ones added later by extensions β€” resolves to the default instead of failing. For the distinct case of an out-of-range integer that matches no declared value, pair it with `UnknownValueImplementation`. The result is that a consumer can assign any enum value to the interface variable and call through it without a runtime guard. + +See sample: `set-defaultimplementation-on-enum.good.al`. + +## Anti Pattern + +An extensible `enum ... implements ` where at least one value sets no `Implementation` and the enum declares no `DefaultImplementation`. Code that assigns that value to an interface variable and invokes a method throws at the call site, and because the enum is extensible the failing value can be introduced by a third party long after the consumer ships. Detection signal: an enum that implements an interface, has a `value(...)` with no `Implementation`, and no enum-level `DefaultImplementation`. Add a `DefaultImplementation` mapping to close the gap. + +See sample: `set-defaultimplementation-on-enum.bad.al`. diff --git a/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.bad.al b/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.bad.al new file mode 100644 index 0000000..164f6bc --- /dev/null +++ b/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.bad.al @@ -0,0 +1,38 @@ +// Intended for read-only consumption, but the CRUD guards are omitted. With +// InsertAllowed/ModifyAllowed/DeleteAllowed left at their writable defaults the +// endpoint silently accepts POST, PATCH, and DELETE, so a client can mutate or +// remove ledger data this API was never meant to expose for writing. +page 50357 "WS Read Only Bad" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'reporting'; + APIVersion = 'v1.0'; + EntityName = 'customerLedgerEntry'; + EntitySetName = 'customerLedgerEntries'; + ODataKeyFields = SystemId; + SourceTable = "Cust. Ledger Entry"; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(entryNumber; Rec."Entry No.") + { + Caption = 'entryNumber'; + } + field(postingDate; Rec."Posting Date") + { + Caption = 'postingDate'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.good.al b/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.good.al new file mode 100644 index 0000000..41e0df0 --- /dev/null +++ b/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.good.al @@ -0,0 +1,39 @@ +page 50356 "WS Read Only Good" +{ + PageType = API; + Caption = 'customerLedgerEntry'; + APIPublisher = 'contoso'; + APIGroup = 'reporting'; + APIVersion = 'v1.0'; + EntityName = 'customerLedgerEntry'; + EntitySetName = 'customerLedgerEntries'; + ODataKeyFields = SystemId; + SourceTable = "Cust. Ledger Entry"; + Editable = false; + InsertAllowed = false; + ModifyAllowed = false; + DeleteAllowed = false; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(entryNumber; Rec."Entry No.") + { + Caption = 'entryNumber'; + } + field(postingDate; Rec."Posting Date") + { + Caption = 'postingDate'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.md b/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.md new file mode 100644 index 0000000..2358a6b --- /dev/null +++ b/microsoft/knowledge/web-services/disable-write-operations-on-read-only-api-pages.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: web-services +keywords: [api-page, insertallowed, modifyallowed, deleteallowed, editable, read-only, reporting-api] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Lock down write operations on read-only API pages + +## Description + +An API meant purely for reading β€” a reporting or lookup endpoint β€” is not read-only just because nobody intends to write to it. Unless the page explicitly forbids writes, the platform leaves the endpoint writable, so a client can POST, PATCH, or DELETE against data that was never meant to change through that surface. The fix is explicit: set `InsertAllowed = false`, `ModifyAllowed = false`, and `DeleteAllowed = false` (and `Editable = false`) so the endpoint rejects every write operation. LLMs often assume "I only exposed read fields, so it's read-only" and rely on defaults; this file is remedial because the default for an API page is writable, and the read-only intent has to be encoded as three explicit property settings, not inferred. + +## Best Practice + +For a read-only / reporting API page set all three CRUD guards off β€” `InsertAllowed = false`, `ModifyAllowed = false`, `DeleteAllowed = false` β€” and mark the page `Editable = false`. The endpoint then serves GET requests and rejects any insert, modify, or delete, matching the read-only contract regardless of the caller. Make the read-only stance explicit rather than depending on the writable default. + +See sample: `disable-write-operations-on-read-only-api-pages.good.al`. + +## Anti Pattern + +An API intended for read-only consumption that omits the CRUD guards, leaving `InsertAllowed`, `ModifyAllowed`, and `DeleteAllowed` at their writable defaults. The endpoint silently accepts POST, PATCH, and DELETE, so a client can mutate or remove data the API was never meant to expose for writing. The detection signal: a read-only/reporting `PageType = API` page that does not set the three `*Allowed = false` properties. + +See sample: `disable-write-operations-on-read-only-api-pages.bad.al`. diff --git a/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.bad.al b/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.bad.al new file mode 100644 index 0000000..df42bdf --- /dev/null +++ b/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.bad.al @@ -0,0 +1,38 @@ +// Committed-only contract, but no isolation level is set. Reads run at the +// default and can observe in-flight, uncommitted writes from concurrent +// transactions. A consumer may fetch a row that is later rolled back β€” a dirty +// read of data that never durably existed. +page 50349 "WS ReadCommitted Bad" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'customer'; + EntitySetName = 'customers'; + ODataKeyFields = SystemId; + SourceTable = Customer; + Editable = false; + InsertAllowed = false; + ModifyAllowed = false; + DeleteAllowed = false; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(displayName; Rec.Name) + { + Caption = 'displayName'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.good.al b/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.good.al new file mode 100644 index 0000000..f619bae --- /dev/null +++ b/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.good.al @@ -0,0 +1,41 @@ +page 50348 "WS ReadCommitted Good" +{ + PageType = API; + Caption = 'customer'; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'customer'; + EntitySetName = 'customers'; + ODataKeyFields = SystemId; + SourceTable = Customer; + Editable = false; + InsertAllowed = false; + ModifyAllowed = false; + DeleteAllowed = false; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(displayName; Rec.Name) + { + Caption = 'displayName'; + } + } + } + } + + trigger OnOpenPage() + begin + // Return only durably committed rows; ignore concurrent uncommitted writes. + Rec.ReadIsolation := IsolationLevel::ReadCommitted; + end; +} diff --git a/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.md b/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.md new file mode 100644 index 0000000..739b5aa --- /dev/null +++ b/microsoft/knowledge/web-services/expose-only-committed-data-from-api-reads.md @@ -0,0 +1,26 @@ +--- +bc-version: [22..] +domain: web-services +keywords: [api-page, readisolation, isolationlevel, readcommitted, onopenpage, dirty-read, committed-data] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Read only committed data from APIs that must not expose in-flight writes + +## Description + +This is about the data-consistency contract of an API endpoint: what a consumer receives when it reads. By default an API read can return in-flight rows that a concurrent, still-open transaction has written but not yet committed. For an endpoint whose contract is "return only data that is durably committed," that is wrong β€” a consumer could fetch a row that the writing transaction later rolls back, then act on data that never really existed. From runtime 22.0 (BC 2023 release wave 1) an API page can pin the isolation level its reads use: setting `Rec.ReadIsolation := IsolationLevel::ReadCommitted;` in the page's `OnOpenPage` trigger makes the endpoint expose only committed rows. LLMs rarely set this on an API page because the platform default "just works" for ordinary UI; this file is remedial because the committed-only endpoint contract requires an explicit opt-in the model would not add on its own. + +## Best Practice + +For an API page that must expose only committed data, set the endpoint's read isolation once as the page opens: in the `OnOpenPage` trigger write `Rec.ReadIsolation := IsolationLevel::ReadCommitted;`. Every read the endpoint then serves ignores uncommitted writes from concurrent transactions, so a consumer never receives a row that another transaction might still roll back. + +See sample: `expose-only-committed-data-from-api-reads.good.al`. + +## Anti Pattern + +An API intended to return committed-only data that sets no isolation level, leaving reads at the default that can observe in-flight, uncommitted writes. A consumer can fetch a row created by a concurrent transaction that is later rolled back β€” a dirty read that surfaces data which never durably existed. The detection signal: a committed-only read API with no `Rec.ReadIsolation := IsolationLevel::ReadCommitted` in `OnOpenPage`. + +See sample: `expose-only-committed-data-from-api-reads.bad.al`. diff --git a/microsoft/knowledge/web-services/expose-operations-as-bound-actions.bad.al b/microsoft/knowledge/web-services/expose-operations-as-bound-actions.bad.al new file mode 100644 index 0000000..bdcdc61 --- /dev/null +++ b/microsoft/knowledge/web-services/expose-operations-as-bound-actions.bad.al @@ -0,0 +1,60 @@ +// Side effect hidden behind a writable flag: PATCHing "posted" to true silently +// triggers posting through OnValidate. The operation is indistinguishable from +// an ordinary data edit and is not discoverable as an action. Expose a +// [ServiceEnabled] bound action instead. +page 50351 "WS Bound Action Bad" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'salesOrder'; + EntitySetName = 'salesOrders'; + ODataKeyFields = SystemId; + SourceTable = "Sales Header"; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(number; Rec."No.") + { + Caption = 'number'; + } + field(posted; IsPosted) + { + Caption = 'posted'; + + trigger OnValidate() + var + PostHelper: Codeunit "WS Bound Action Bad Helper"; + begin + if IsPosted then + PostHelper.PostOrder(Rec); + end; + } + } + } + } + + var + IsPosted: Boolean; +} + +codeunit 50353 "WS Bound Action Bad Helper" +{ + procedure PostOrder(var SalesHeader: Record "Sales Header") + var + SalesPost: Codeunit "Sales-Post"; + begin + SalesPost.Run(SalesHeader); + end; +} diff --git a/microsoft/knowledge/web-services/expose-operations-as-bound-actions.good.al b/microsoft/knowledge/web-services/expose-operations-as-bound-actions.good.al new file mode 100644 index 0000000..236e766 --- /dev/null +++ b/microsoft/knowledge/web-services/expose-operations-as-bound-actions.good.al @@ -0,0 +1,59 @@ +page 50350 "WS Bound Action Good" +{ + PageType = API; + Caption = 'salesOrder'; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'salesOrder'; + EntitySetName = 'salesOrders'; + ODataKeyFields = SystemId; + SourceTable = "Sales Header"; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(number; Rec."No.") + { + Caption = 'number'; + } + } + } + } + + [ServiceEnabled] + procedure Post(var ActionContext: WebServiceActionContext) + var + PostHelper: Codeunit "WS Bound Action Helper"; + begin + PostHelper.PostOrder(Rec); + SetActionResponse(ActionContext, Rec.SystemId); + end; + + local procedure SetActionResponse(var ActionContext: WebServiceActionContext; CreatedId: Guid) + begin + ActionContext.SetObjectType(ObjectType::Page); + ActionContext.SetObjectId(Page::"WS Bound Action Good"); + ActionContext.AddEntityKey(Rec.FieldNo(SystemId), CreatedId); + ActionContext.SetResultCode(WebServiceActionResultCode::Updated); + end; +} + +codeunit 50352 "WS Bound Action Helper" +{ + procedure PostOrder(var SalesHeader: Record "Sales Header") + var + SalesPost: Codeunit "Sales-Post"; + begin + SalesPost.Run(SalesHeader); + end; +} diff --git a/microsoft/knowledge/web-services/expose-operations-as-bound-actions.md b/microsoft/knowledge/web-services/expose-operations-as-bound-actions.md new file mode 100644 index 0000000..7f0ed87 --- /dev/null +++ b/microsoft/knowledge/web-services/expose-operations-as-bound-actions.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: web-services +keywords: [api-page, serviceenabled, bound-action, webserviceactioncontext, setactionresponse, side-effect, patch] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Expose business operations as bound actions, not as writable status flags + +## Description + +An API consumer that needs to *do* something to a record β€” post it, ship it, release it β€” should call an explicit operation, not mutate a field and hope a side effect fires. AL models this with a bound action: a `[ServiceEnabled] procedure` that takes `var ActionContext: WebServiceActionContext`, performs the work, and reports the result through the action context (typically a `SetActionResponse` helper that returns the affected record's id). The endpoint then exposes a callable action β€” `.../salesOrders()/Microsoft.NAV.post` β€” with a clear contract. The anti-pattern is to expose a writable Boolean or status field whose `OnValidate` quietly performs the operation: a routine PATCH that looks like a data edit silently triggers posting, with no discoverable action and surprising, hard-to-audit behaviour. LLMs reach for the flag-field approach because it is less code; this file is remedial because the platform-idiomatic, contract-safe choice (a bound action) is not the model's default. + +## Best Practice + +Declare the operation as `[ServiceEnabled] procedure Post(var ActionContext: WebServiceActionContext)` on the API page. Inside, perform the operation against `Rec`, then call a `SetActionResponse` helper that writes the result β€” the bound record and its id β€” back into the `WebServiceActionContext` so the caller receives a well-formed response. The operation is now an explicit, named endpoint action separate from ordinary field writes. + +See sample: `expose-operations-as-bound-actions.good.al`. + +## Anti Pattern + +Exposing a writable Boolean (for example `posted`) whose `OnValidate` performs the posting. A client that PATCHes the field to `true` β€” an action indistinguishable from any other data edit β€” silently triggers a side-effecting business operation. The detection signal: an API page field whose `OnValidate` posts, ships, or releases, instead of a `[ServiceEnabled]` bound action. + +See sample: `expose-operations-as-bound-actions.bad.al`. diff --git a/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.bad.al b/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.bad.al new file mode 100644 index 0000000..c7f7e2d --- /dev/null +++ b/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.bad.al @@ -0,0 +1,33 @@ +// Unstable key: the endpoint addresses records by the business field "No.". +// When a user renames a customer's number, every external reference built on +// the old value dangles. ODataKeyFields should be SystemId instead. +page 50345 "WS SystemId Key Bad" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'customer'; + EntitySetName = 'customers'; + ODataKeyFields = "No."; + SourceTable = Customer; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(number; Rec."No.") + { + Caption = 'number'; + } + field(displayName; Rec.Name) + { + Caption = 'displayName'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.good.al b/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.good.al new file mode 100644 index 0000000..e22015d --- /dev/null +++ b/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.good.al @@ -0,0 +1,36 @@ +page 50344 "WS SystemId Key Good" +{ + PageType = API; + Caption = 'customer'; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'customer'; + EntitySetName = 'customers'; + ODataKeyFields = SystemId; + SourceTable = Customer; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(number; Rec."No.") + { + Caption = 'number'; + } + field(displayName; Rec.Name) + { + Caption = 'displayName'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md b/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md new file mode 100644 index 0000000..93d2dcd --- /dev/null +++ b/microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: web-services +keywords: [api-page, odatakeyfields, systemid, stable-key, guid, business-key, editable-false] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Address API records by SystemId, not by a renamable business key + +## Description + +Every BC table carries a `SystemId` β€” an immutable GUID assigned at insert and never reused. API consumers must address a record through a key that does not change, otherwise a previously stored URL or `@odata.id` reference breaks the moment a user renames the underlying business key. The convention is to set `ODataKeyFields = SystemId` on the API page and expose the GUID as a non-editable `field(id; Rec.SystemId)`. An LLM left to its own devices often reaches for the human-readable primary key (a customer `No.`, an item code) as the OData key, because that is what a developer types when filtering in AL. That choice is wrong for an external contract: business keys are renamable and the API caller's stored references would dangle. This file is remedial because the correct key (`SystemId`) is rarely the one the model would pick by analogy with ordinary AL code. + +## Best Practice + +Set `ODataKeyFields = SystemId` so OData routes records by the stable GUID, and expose it as `field(id; Rec.SystemId)` marked `Editable = false`. Clients then address a record at `.../customers()`, an identity that survives any rename of the business key. Keep the business key (for example `No.`) as an ordinary exposed field, not as the OData key. + +See sample: `expose-systemid-as-the-api-key.good.al`. + +## Anti Pattern + +Setting `ODataKeyFields = "No."` so the endpoint addresses records by a renamable business field. As soon as a user changes that `No.`, every external reference built on the old value points at nothing, silently breaking integrations. The detection signal: `ODataKeyFields` set to a business field rather than `SystemId`, or an API page that exposes no `id` field bound to `Rec.SystemId`. + +See sample: `expose-systemid-as-the-api-key.bad.al`. diff --git a/microsoft/knowledge/web-services/set-required-api-page-properties.bad.al b/microsoft/knowledge/web-services/set-required-api-page-properties.bad.al new file mode 100644 index 0000000..927bc2d --- /dev/null +++ b/microsoft/knowledge/web-services/set-required-api-page-properties.bad.al @@ -0,0 +1,24 @@ +// Malformed API endpoint: APIPublisher and APIGroup are missing, and there is +// no SourceTable. The page compiles but the route cannot be composed, so the +// entity is never published where an integration expects it. +page 50341 "WS Required Props Bad" +{ + PageType = API; + APIVersion = 'v1.0'; + EntityName = 'customer'; + EntitySetName = 'customers'; + + layout + { + area(content) + { + repeater(records) + { + field(displayName; Rec.Name) + { + Caption = 'displayName'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/set-required-api-page-properties.good.al b/microsoft/knowledge/web-services/set-required-api-page-properties.good.al new file mode 100644 index 0000000..1055ed6 --- /dev/null +++ b/microsoft/knowledge/web-services/set-required-api-page-properties.good.al @@ -0,0 +1,36 @@ +page 50340 "WS Required Props Good" +{ + PageType = API; + Caption = 'customer'; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'customer'; + EntitySetName = 'customers'; + ODataKeyFields = SystemId; + SourceTable = Customer; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(number; Rec."No.") + { + Caption = 'number'; + } + field(displayName; Rec.Name) + { + Caption = 'displayName'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/set-required-api-page-properties.md b/microsoft/knowledge/web-services/set-required-api-page-properties.md new file mode 100644 index 0000000..9bef346 --- /dev/null +++ b/microsoft/knowledge/web-services/set-required-api-page-properties.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: web-services +keywords: [api-page, pagetype-api, apipublisher, apigroup, apiversion, entityname, entitysetname, sourcetable] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Declare every required property on a PageType = API page + +## Description + +An API page projects a table as an OData v4 / API v2 endpoint, but the platform only publishes that endpoint when the page carries the full set of identifying properties: `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, and a backing `SourceTable`. These properties are what compose the route β€” `/api////` β€” so omitting any one of them yields a page that compiles yet never surfaces as a usable endpoint, or surfaces at an unexpected address. An LLM that has mostly seen ordinary list/card pages tends to treat `PageType = API` as a cosmetic switch and forgets the identifying metadata, because a normal page needs none of it. This file is remedial precisely because the missing-property failure is silent: there is no runtime error, only an endpoint that clients cannot reach. + +## Best Practice + +On every `PageType = API` page set all six properties explicitly: `APIPublisher` (your publisher tag), `APIGroup` (the logical grouping for related entities), `APIVersion` (a `vX.Y` value such as `'v1.0'`), `EntityName` (singular), `EntitySetName` (plural), and `SourceTable` (the projected table). Expose the record's fields inside a single `field(...)` repeater under `area(content)`. Treat the six properties as a mandatory checklist that travels with the `PageType = API` declaration itself. + +See sample: `set-required-api-page-properties.good.al`. + +## Anti Pattern + +Writing a page with `PageType = API` and a `SourceTable` but leaving out `APIPublisher` and `APIGroup` (and, worse, omitting `SourceTable` entirely). The page compiles, so it looks finished, but the endpoint is malformed: with no publisher and group the route cannot be composed, and the entity is never published where an integration expects it. The detection signal: a `PageType = API` page missing one or more of the six identifying properties. + +See sample: `set-required-api-page-properties.bad.al`. diff --git a/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.bad.al b/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.bad.al new file mode 100644 index 0000000..ccea85b --- /dev/null +++ b/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.bad.al @@ -0,0 +1,35 @@ +// Breaking change in place: the published v1.0 is edited rather than versioned. +// EntityName was renamed from 'customer' to 'client' and the displayName field +// was removed, so the single declared version now serves a different contract +// than the one clients integrated against. Every existing consumer breaks. +page 50355 "WS API Versioning Bad" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + EntityName = 'client'; + EntitySetName = 'clients'; + ODataKeyFields = SystemId; + SourceTable = Customer; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(number; Rec."No.") + { + Caption = 'number'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.good.al b/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.good.al new file mode 100644 index 0000000..97aeb3a --- /dev/null +++ b/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.good.al @@ -0,0 +1,39 @@ +// Additive versioning: v2.0 carries the new shape while v1.0 stays published and +// unchanged. APIVersion accepts a list, so both contracts are served and +// existing clients keep working while new clients adopt v2.0. +page 50354 "WS API Versioning Good" +{ + PageType = API; + Caption = 'customer'; + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v2.0', 'v1.0'; + EntityName = 'customer'; + EntitySetName = 'customers'; + ODataKeyFields = SystemId; + SourceTable = Customer; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) + { + Caption = 'id'; + Editable = false; + } + field(number; Rec."No.") + { + Caption = 'number'; + } + field(displayName; Rec.Name) + { + Caption = 'displayName'; + } + } + } + } +} diff --git a/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.md b/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.md new file mode 100644 index 0000000..af998ed --- /dev/null +++ b/microsoft/knowledge/web-services/version-apis-by-adding-not-mutating-published-versions.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: web-services +keywords: [api-page, apiversion, versioning, published-contract, breaking-change, backward-compatibility] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Version APIs by adding a new APIVersion, not by mutating a published one + +## Description + +Once an API version is published, external clients depend on its exact shape β€” the entity name, the set of exposed fields, the key β€” as a frozen contract. Changing any of that on the already-published version is a breaking change delivered silently: integrations that worked yesterday fail today with no warning. The platform gives you a clean way to evolve without breaking anyone, because `APIVersion` accepts a *list* of versions on one page. The correct way to change a published API is to add the new version (`'v2.0'`) alongside the existing one (`'v1.0'`) β€” or publish a new API page for it β€” so both contracts are served side by side and clients migrate on their own schedule. LLMs tend to "fix" an API by editing the live version in place, because in ordinary code you just change what's wrong; this file is remedial because a published API version is an immutable contract in a way ordinary internal code is not. + +## Best Practice + +When a published API must change shape, keep the old version's contract intact and add the new one to the `APIVersion` list β€” `APIVersion = 'v2.0', 'v1.0';`. The page now serves both `v1.0` (unchanged) and `v2.0` (carrying the new shape), so existing clients keep working while new clients adopt `v2.0`. Retire the old version only after consumers have migrated. + +See sample: `version-apis-by-adding-not-mutating-published-versions.good.al`. + +## Anti Pattern + +Editing the published `v1.0` page in place β€” renaming its `EntityName` or removing an exposed field β€” so the single declared version now serves a different contract than the one clients integrated against. Every consumer of the old shape breaks without notice. The detection signal: a change that renames the entity or removes a field on an existing published `APIVersion` instead of adding a new version to the list. + +See sample: `version-apis-by-adding-not-mutating-published-versions.bad.al`. diff --git a/microsoft/skills/review/al-breaking-changes-review.md b/microsoft/skills/review/al-breaking-changes-review.md new file mode 100644 index 0000000..4238bca --- /dev/null +++ b/microsoft/skills/review/al-breaking-changes-review.md @@ -0,0 +1,136 @@ +--- +kind: action-skill +id: al-breaking-changes-review +version: 1 +title: AL breaking changes review +description: Reviews AL source changes against breaking-changes guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL breaking changes review + +Reviews AL source changes against the `breaking-changes` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. + +An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once β€” the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone β€” see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint β€” exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `breaking-changes` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/breaking-changes/**`. + +## Relevance + +Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context: + +- `bc-version` β€” the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`. +- `technologies` β€” `[al]`. +- `countries` β€” the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`. +- `application-area` β€” the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: + +- The changed AL object names and types β€” especially codeunits, tables, and table extensions that expose procedures, fields, or events to other apps, and any member whose access is being widened. +- The changed procedures, fields, and triggers, weighted toward non-`local` procedures, published table fields, event publishers, and any member whose signature, access modifier, or obsolete state is being altered. +- Tokens extracted from the diff that relate to API stability and deprecation (`signature`, `parameter`, `return`, `var`, `Obsolete`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `Pending`, `Removed`, `CLEAN`, `SecretText`, `token`, `internal`, `local`, `public`, `protected`, `Scope`). + +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. + +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`. + +When the post-conflict worklist is empty because no applicable breaking-changes knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable breaking-changes knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array. + +## Action + +For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: + +- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`. +- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. +- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`. + +Set `confidence` to: + +- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type). +- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. +- `low` when the finding is an advisory derived only from applicability. + +After evaluating each worklist entry, also consider whether the diff exhibits a breaking-changes defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain β€” emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material API-stability defect a knowledgeable BC reviewer would agree is wrong β€” steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly breaking changes; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate β€” if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. + +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: restore a published procedure's parameter list and add a new overload for the extra argument; add an `[Obsolete]` attribute to a member being removed; change a needlessly `public` helper to `internal`). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement β€” no diff markers, no fences, no commentary β€” that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. + +Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. + +Outcome selection: + +- `completed` β€” the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty. +- `no-knowledge` β€” no applicable breaking-changes knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty. +- `not-applicable` β€” the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task). +- `partial` β€” a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause. +- `failed` β€” an unrecoverable error occurred. `outcome-reason` is required. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-breaking-changes-review", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md", + "severity": "major", + "message": "A parameter was added to the published procedure CalculateDiscount, breaking every dependent extension that called the previous form. Add a new overload alongside the unchanged procedure instead.", + "location": { + "file": "src/Sales/DiscountApi.Codeunit.al", + "line": 12, + "range": { "start-line": 12, "end-line": 15 } + }, + "references": [ + { "path": "microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md" } + ], + "confidence": "high" + }, + { + "id": "microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md", + "severity": "minor", + "message": "An implementation-detail helper is declared public with no reason to support it externally, making it a de-facto API. Default it to internal or local.", + "location": { + "file": "src/Sales/OrderProcessor.Codeunit.al", + "line": 20 + }, + "references": [ + { "path": "microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md" } + ], + "confidence": "medium" + } + ], + "suppressed": [] +} +``` + +The empty-corpus case β€” BCQuality's state until breaking-changes knowledge files land β€” produces: + +```json +{ + "skill": { "id": "al-breaking-changes-review", "version": 1 }, + "outcome": "no-knowledge", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 0, "items-evaluated": 0 } + }, + "findings": [], + "suppressed": [] +} +``` diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index f54ff2b..ba58d70 100644 --- a/microsoft/skills/review/al-code-review.md +++ b/microsoft/skills/review/al-code-review.md @@ -3,7 +3,7 @@ kind: action-skill id: al-code-review version: 1 title: AL code review -description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI). +description: Reviews AL source changes by composing the AL review leaf skills, one per knowledge domain. inputs: [pr-diff, file-path] outputs: [findings-report] bc-version: [all] @@ -17,6 +17,11 @@ sub-skills: - microsoft/skills/review/al-upgrade-review.md - microsoft/skills/review/al-style-review.md - microsoft/skills/review/al-ui-review.md + - microsoft/skills/review/al-error-handling-review.md + - microsoft/skills/review/al-events-review.md + - microsoft/skills/review/al-interfaces-review.md + - microsoft/skills/review/al-breaking-changes-review.md + - microsoft/skills/review/al-web-services-review.md --- # AL code review @@ -29,16 +34,7 @@ An orchestrator invokes this skill with either a `pr-diff` (the standard PR-revi ## Source -The sub-skills invoked by this skill are those listed in frontmatter `sub-skills`: - -- `microsoft/skills/review/al-performance-review.md` -- `microsoft/skills/review/al-security-review.md` -- `microsoft/skills/review/al-privacy-review.md` -- `microsoft/skills/review/al-upgrade-review.md` -- `microsoft/skills/review/al-style-review.md` -- `microsoft/skills/review/al-ui-review.md` - -Additional leaf skills (for example, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly. +The sub-skills invoked by this skill are those listed in frontmatter `sub-skills`. Additional leaf skills (for example, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly. ## Relevance diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md new file mode 100644 index 0000000..91228aa --- /dev/null +++ b/microsoft/skills/review/al-error-handling-review.md @@ -0,0 +1,136 @@ +--- +kind: action-skill +id: al-error-handling-review +version: 1 +title: AL error handling review +description: Reviews AL source changes against error-handling guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL error handling review + +Reviews AL source changes against the `error-handling` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. + +An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once β€” the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone β€” see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint β€” exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `error-handling` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/error-handling/**`. + +## Relevance + +Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context: + +- `bc-version` β€” the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`. +- `technologies` β€” `[al]`. +- `countries` β€” the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`. +- `application-area` β€” the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: + +- The changed AL object names and types β€” especially codeunits that post or validate, tables and table extensions with `OnValidate` triggers, and any procedure that raises errors or orchestrates a batch over records. +- The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]`. +- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`). + +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. + +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`. + +When the post-conflict worklist is empty because no applicable error-handling knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable error-handling knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array. + +## Action + +For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: + +- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`. +- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. +- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`. + +Set `confidence` to: + +- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type). +- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. +- `low` when the finding is an advisory derived only from applicability. + +After evaluating each worklist entry, also consider whether the diff exhibits an error-handling defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain β€” emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material error-handling defect a knowledgeable BC reviewer would agree is wrong β€” steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly error handling; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate β€” if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. + +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: replace a string-concatenated `Error` with a Label-backed call; mark an internal-only failure `ErrorType::Internal`; add a missing `DetailedMessage`). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement β€” no diff markers, no fences, no commentary β€” that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. + +Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. + +Outcome selection: + +- `completed` β€” the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty. +- `no-knowledge` β€” no applicable error-handling knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty. +- `not-applicable` β€” the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task). +- `partial` β€” a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause. +- `failed` β€” an unrecoverable error occurred. `outcome-reason` is required. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-error-handling-review", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md", + "severity": "major", + "message": "A validation error names the maximum allowed quantity but raises a plain Error with no recommended action. Use an ErrorInfo with a Fix-it AddAction so the user can apply the known value.", + "location": { + "file": "src/Sales/SalesLine.TableExt.al", + "line": 88, + "range": { "start-line": 86, "end-line": 89 } + }, + "references": [ + { "path": "microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md" } + ], + "confidence": "high" + }, + { + "id": "microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md", + "severity": "minor", + "message": "This 'unexpected state' failure is developer-facing but is raised with default Client visibility. Mark it ErrorType::Internal so the detail goes to telemetry and the user sees a generic message.", + "location": { + "file": "src/Ledger/PostingEngine.Codeunit.al", + "line": 211 + }, + "references": [ + { "path": "microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md" } + ], + "confidence": "medium" + } + ], + "suppressed": [] +} +``` + +The empty-corpus case β€” BCQuality's state until error-handling knowledge files land β€” produces: + +```json +{ + "skill": { "id": "al-error-handling-review", "version": 1 }, + "outcome": "no-knowledge", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 0, "items-evaluated": 0 } + }, + "findings": [], + "suppressed": [] +} +``` diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md new file mode 100644 index 0000000..0fe9479 --- /dev/null +++ b/microsoft/skills/review/al-events-review.md @@ -0,0 +1,153 @@ +--- +kind: action-skill +id: al-events-review +version: 1 +title: AL events review +description: Reviews AL source changes against events-and-subscribers guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL events review + +Reviews AL source changes against the `events` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. + +An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once β€” the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone β€” see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint β€” exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `events` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/events/**`. + +## Relevance + +Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context: + +- `bc-version` β€” the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`. +- `technologies` β€” `[al]`. +- `countries` β€” the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`. +- `application-area` β€” the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: + +- The changed AL object names and types β€” especially codeunits that publish events or host event subscribers, posting/release/validation routines that should expose extension points, and test codeunits that bind subscribers. +- The changed procedures and triggers, weighted toward event publisher methods, methods carrying the `[EventSubscriber(...)]` attribute, routines that raise `OnBefore`/`OnAfter` events, and any procedure that calls `BindSubscription`/`UnbindSubscription`. +- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`). + +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. + +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`. + +When the post-conflict worklist is empty because no applicable events knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable events knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array. + +### Event-design checks + +The following targeted checks map diff signals to specific `events` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action. + +- `IsHandled` raised without an immediately preceding `IsHandled := false;`, or one `IsHandled` variable reused across several raises with no reset between them β€” `initialize-ishandled-to-false-before-publishing`. +- `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event later, so the after-event is skipped whenever the call is handled β€” `preserve-onafter-execution-when-ishandled-skips-the-body`. +- A parameter added before existing parameters on a changed event signature instead of appended at the end β€” `add-new-event-parameters-at-the-end`. +- Publisher names that do not encode firing position (`OnBefore`/`OnAfter` at the boundaries, `OnOnBefore`/`OnAfter` mid-routine) β€” `name-events-by-publisher-position`. +- Two consecutive `OnBefore`/`OnAfter` raises with no logic between them, or a near-duplicate event differing only by an extra parameter β€” `prefer-reusing-or-extending-existing-events`. +- An event raised between `repeat` and `until` inside a record loop β€” `do-not-publish-events-inside-loops`. +- A `temporary` record event parameter whose name does not start with `Temp` β€” `prefix-temporary-record-event-parameters-with-temp`. +- Abbreviated event parameter names (`SalesHdr`, `DocNo`, `Amt`) instead of full table names and spelled-out values β€” `name-event-parameters-without-abbreviations`. +- `[IntegrationEvent(true, …)]` (`IncludeSender`) on a codeunit event used only to expose the publisher, where `this` could be passed as a typed `Sender` parameter (Business Central 2024 release wave 2 and later) β€” `prefer-this-over-includesender-in-codeunit-events`. +- A `RecordRef` event parameter, or a passed-through `xRec`, where a concrete typed record fits β€” `avoid-loosely-typed-event-parameters`. +- A `var IsHandled` added to a pre-existing event rather than introduced through a new `OnBefore` publisher β€” `do-not-add-ishandled-to-an-existing-event`. +- An `if IsHandled then exit;` whose skipped body performs posting, ledger-entry creation, number-series consumption, or integrity/permission validation β€” `do-not-bypass-critical-operations-with-ishandled`. + +## Action + +For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: + +- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`. +- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. +- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`. + +Set `confidence` to: + +- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type). +- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. +- `low` when the finding is an advisory derived only from applicability. + +After evaluating each worklist entry, also consider whether the diff exhibits an events defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain β€” emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material events defect a knowledgeable BC reviewer would agree is wrong β€” steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly events and subscribers; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate β€” if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. + +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: empty out a non-empty `[IntegrationEvent]` publisher body; add the missing `if IsHandled then exit;` guard after an `OnBefore` raise; add a matching `UnbindSubscription` for a leaked `BindSubscription`; set `EventSubscriberInstance = Manual;` on a codeunit that must be scoped). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement β€” no diff markers, no fences, no commentary β€” that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. + +Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. + +Outcome selection: + +- `completed` β€” the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty. +- `no-knowledge` β€” no applicable events knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty. +- `not-applicable` β€” the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task). +- `partial` β€” a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause. +- `failed` β€” an unrecoverable error occurred. `outcome-reason` is required. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-events-review", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md", + "severity": "major", + "message": "Business logic is placed inside an [IntegrationEvent] publisher body, so the event mutates state on every raise instead of being a thin hook. Move the logic into the calling routine and leave the publisher body empty.", + "location": { + "file": "src/Sales/ReservationMgt.Codeunit.al", + "line": 64, + "range": { "start-line": 61, "end-line": 67 } + }, + "references": [ + { "path": "microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md" } + ], + "confidence": "high" + }, + { + "id": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md", + "severity": "minor", + "message": "An OnBefore event is raised with a var IsHandled parameter, but the routine never guards with 'if IsHandled then exit;', so the default logic still runs after a subscriber handled the call.", + "location": { + "file": "src/Sales/ReservationMgt.Codeunit.al", + "line": 41 + }, + "references": [ + { "path": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md" } + ], + "confidence": "high" + } + ], + "suppressed": [] +} +``` + +The empty-corpus case β€” BCQuality's state until events knowledge files land β€” produces: + +```json +{ + "skill": { "id": "al-events-review", "version": 1 }, + "outcome": "no-knowledge", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 0, "items-evaluated": 0 } + }, + "findings": [], + "suppressed": [] +} +``` diff --git a/microsoft/skills/review/al-interfaces-review.md b/microsoft/skills/review/al-interfaces-review.md new file mode 100644 index 0000000..c859b75 --- /dev/null +++ b/microsoft/skills/review/al-interfaces-review.md @@ -0,0 +1,136 @@ +--- +kind: action-skill +id: al-interfaces-review +version: 1 +title: AL interfaces review +description: Reviews AL source changes against interface and enum-with-implementation guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL interfaces review + +Reviews AL source changes against the `interfaces` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. + +An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once β€” the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone β€” see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint β€” exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `interfaces` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/interfaces/**`. + +## Relevance + +Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context: + +- `bc-version` β€” the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. Interface guidance is gated at Business Central 2020 release wave 1 (BC16), so a target below 16 discards it. If unavailable, the dimension is `unknown`. +- `technologies` β€” `[al]`. +- `countries` β€” the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`. +- `application-area` β€” the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: + +- The changed AL object names and types β€” especially `interface` objects, codeunits and enums declared with the `implements` keyword, and consumers that declare or assign an `Interface` variable. +- The changed procedures and triggers, weighted toward factory or dispatch routines that resolve a variant to behaviour, setter-injection procedures that take an `Interface` parameter, and `case`-over-enum blocks that select between strategies. +- Tokens extracted from the diff that relate to interfaces and enum-backed implementation (`interface`, `implements`, `Implementation`, `DefaultImplementation`, `UnknownValueImplementation`, `enum`, `Extensible`, `Interface`, `case`, and the `case of` anti-pattern signal β€” a `case` over an enum value whose branches choose between variant computations). + +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. + +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`. + +When the post-conflict worklist is empty because no applicable interfaces knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable interfaces knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array. + +## Action + +For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: + +- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`. +- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. +- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`. + +Set `confidence` to: + +- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type). +- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. +- `low` when the finding is an advisory derived only from applicability. + +After evaluating each worklist entry, also consider whether the diff exhibits an interfaces defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain β€” emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material interfaces defect a knowledgeable BC reviewer would agree is wrong β€” steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly interfaces and enum-with-implementation; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate β€” if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. + +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: add a `DefaultImplementation` mapping to an extensible enum that implements an interface; add the `Implementation` property to a new enum value; change a concrete `Codeunit` collaborator variable to its `Interface` type). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement β€” no diff markers, no fences, no commentary β€” that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. + +Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. + +Outcome selection: + +- `completed` β€” the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty. +- `no-knowledge` β€” no applicable interfaces knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty. +- `not-applicable` β€” the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task). +- `partial` β€” a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause. +- `failed` β€” an unrecoverable error occurred. `outcome-reason` is required. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-interfaces-review", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md", + "severity": "major", + "message": "Behaviour is selected with a 'case' over the Shipping Method enum, and the same shape is duplicated in a second procedure. Model the enum as one that implements an interface and dispatch through an interface variable so new methods do not edit every call site.", + "location": { + "file": "src/Shipping/ShippingCharge.Codeunit.al", + "line": 22, + "range": { "start-line": 22, "end-line": 31 } + }, + "references": [ + { "path": "microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md" } + ], + "confidence": "high" + }, + { + "id": "microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md", + "severity": "minor", + "message": "This extensible enum implements an interface but the 'None' value sets no Implementation and the enum declares no DefaultImplementation. Resolving 'None' to the interface and calling a method will fail at runtime. Add a DefaultImplementation mapping.", + "location": { + "file": "src/Notifications/NotificationChannel.Enum.al", + "line": 9 + }, + "references": [ + { "path": "microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md" } + ], + "confidence": "high" + } + ], + "suppressed": [] +} +``` + +The empty-corpus case β€” BCQuality's state before interfaces knowledge files land β€” produces: + +```json +{ + "skill": { "id": "al-interfaces-review", "version": 1 }, + "outcome": "no-knowledge", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 0, "items-evaluated": 0 } + }, + "findings": [], + "suppressed": [] +} +``` diff --git a/microsoft/skills/review/al-web-services-review.md b/microsoft/skills/review/al-web-services-review.md new file mode 100644 index 0000000..4109722 --- /dev/null +++ b/microsoft/skills/review/al-web-services-review.md @@ -0,0 +1,136 @@ +--- +kind: action-skill +id: al-web-services-review +version: 1 +title: AL web services review +description: Reviews AL source changes against web-services (API page) guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL web services review + +Reviews AL source changes against the `web-services` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. + +An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once β€” the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone β€” see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint β€” exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `web-services` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/web-services/**`. + +## Relevance + +Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context: + +- `bc-version` β€” the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`. +- `technologies` β€” `[al]`. +- `countries` β€” the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`. +- `application-area` β€” the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: + +- The changed AL object names and types β€” especially page objects declared with `PageType = API`, and any procedure on such a page that exposes a bound action. +- The changed properties and triggers, weighted toward API page metadata (`APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SourceTable`), CRUD guards (`InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`), the `OnOpenPage` trigger, and `OnValidate` triggers on exposed fields. +- Tokens extracted from the diff that relate to API surface and behaviour (`PageType`, `API`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SystemId`, `ServiceEnabled`, `WebServiceActionContext`, `SetActionResponse`, `ReadIsolation`, `IsolationLevel`, `ReadCommitted`, `InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`, `SourceTable`). + +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. + +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`. + +When the post-conflict worklist is empty because no applicable web-services knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable web-services knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array. + +## Action + +For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: + +- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`. +- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. +- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`. + +Set `confidence` to: + +- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type). +- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. +- `low` when the finding is an advisory derived only from applicability. + +After evaluating each worklist entry, also consider whether the diff exhibits a web-services defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain β€” emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material web-services defect a knowledgeable BC reviewer would agree is wrong β€” steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly API pages and web-service surfaces; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate β€” if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. + +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: set `ODataKeyFields = SystemId`; add the three `*Allowed = false` guards to a read-only page; add the missing `OnOpenPage` isolation assignment). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement β€” no diff markers, no fences, no commentary β€” that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. + +Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. + +Outcome selection: + +- `completed` β€” the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty. +- `no-knowledge` β€” no applicable web-services knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty. +- `not-applicable` β€” the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task). +- `partial` β€” a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause. +- `failed` β€” an unrecoverable error occurred. `outcome-reason` is required. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-web-services-review", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "microsoft/knowledge/web-services/set-required-api-page-properties.md", + "severity": "major", + "message": "This PageType = API page declares a SourceTable but omits APIPublisher and APIGroup, so the endpoint route cannot be composed and the entity is never published. Declare all six required API page properties.", + "location": { + "file": "src/Api/CustomerApi.Page.al", + "line": 3, + "range": { "start-line": 1, "end-line": 8 } + }, + "references": [ + { "path": "microsoft/knowledge/web-services/set-required-api-page-properties.md" } + ], + "confidence": "high" + }, + { + "id": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md", + "severity": "minor", + "message": "This API page sets ODataKeyFields to a renamable business field instead of SystemId, so stored references break when the business key changes. Set ODataKeyFields = SystemId and expose field(id; Rec.SystemId).", + "location": { + "file": "src/Api/CustomerApi.Page.al", + "line": 9 + }, + "references": [ + { "path": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md" } + ], + "confidence": "high" + } + ], + "suppressed": [] +} +``` + +The empty-corpus case β€” when no web-services knowledge survives filtering β€” produces: + +```json +{ + "skill": { "id": "al-web-services-review", "version": 1 }, + "outcome": "no-knowledge", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 0, "items-evaluated": 0 } + }, + "findings": [], + "suppressed": [] +} +``` diff --git a/skills/do.md b/skills/do.md index 317750d..777f89f 100644 --- a/skills/do.md +++ b/skills/do.md @@ -117,6 +117,12 @@ Every action skill emits a single JSON document that conforms to this schema: } ``` +### JSON validity + +The emitted document MUST be strict, valid JSON per [RFC 8259](https://www.rfc-editor.org/rfc/rfc8259). Inside every string value, all double quotes MUST be escaped as `\"` and all line breaks as `\n`; other control characters MUST use their JSON escapes. This is not optional polish β€” it is the difference between a parseable report and one a consumer silently drops. + +AL source is the common failure case. Quoted identifiers (for example `Rec."No."`) and multi-line snippets routinely appear in `message`, `suggested-code`, and `suggested-code-omission-reason`, and each embedded quote or newline MUST be escaped when placed in a string value. A `suggested-code` payload that spans several lines is a single JSON string with `\n` separators, not a literal multi-line block. Emit the document as one JSON value with no trailing commentary, and do not rely on the consumer to repair unescaped output. + ### Field semantics **`outcome`** (required) β€” diff --git a/skills/read.md b/skills/read.md index dbeaa63..8badb97 100644 --- a/skills/read.md +++ b/skills/read.md @@ -26,7 +26,7 @@ A file that violates any of these rules is invalid and MUST be skipped by consum ```yaml --- -bc-version: [all] # or [26, 27, 28] or the range shorthand [26..28] +bc-version: [all] # or [26, 27, 28], the range [26..28], or the open-ended range [26..] domain: performance keywords: [query, filtering, partial] technologies: [al] @@ -39,13 +39,14 @@ All six fields are required. Missing or empty fields invalidate the file. ### Fields -**`bc-version`** β€” Array. The Business Central major versions this file applies to. Three forms are accepted: +**`bc-version`** β€” Array. The Business Central major versions this file applies to. Four forms are accepted: - Universal sentinel: `[all]` means the guidance applies to every BC version and matches any target. - Explicit list: `[26, 27, 28]`. -- Range shorthand: `[26..28]` means every integer from 26 through 28 inclusive. +- Closed range shorthand: `[26..28]` means every integer from 26 through 28 inclusive. +- Open-ended range shorthand: `[26..]` means version 26 and every later version, with no upper bound. Use it for guidance tied to a feature introduced in a specific version that is not expected to be removed. -`[all]` is mutually exclusive with explicit versions; do not combine. Consumers MUST expand ranges to the full set before comparison. +`[all]` is mutually exclusive with explicit versions; do not combine. Consumers MUST expand closed ranges to the full set before comparison; an open-ended range `[N..]` is not enumerable and instead matches any target version greater than or equal to `N`. **`domain`** β€” String. A single domain tag that places the file within a broader area of concern. Standard values include `performance`, `security`, `ux`, `telemetry`, `testing`, `api`, `pipelines`, `finance`, `supply-chain`, `manufacturing`, `jobs`. New domains may be introduced by contributors; no closed enumeration is enforced at the schema level. Consumers MUST treat unknown domains as valid. @@ -94,7 +95,7 @@ Conflict detection is the consumer's responsibility; BCQuality does not enforce When a consumer filters or matches files against a task context, these rules apply: -- **`bc-version`** β€” the file matches if its set is `[all]`, or if the target BC version is an element of the file's expanded `bc-version` set. Range shorthand (`[26..28]`) MUST be expanded before comparison. +- **`bc-version`** β€” the file matches if its set is `[all]`, or if the target BC version is an element of the file's expanded `bc-version` set. Closed range shorthand (`[26..28]`) MUST be expanded before comparison; an open-ended range (`[26..]`) matches when the target BC version is greater than or equal to its lower bound. - **`technologies`** β€” non-empty intersection between the task's technologies and the file's technologies. There is no sentinel for this field. - **`countries`** β€” the file matches if its set contains `w1`, or if there is a non-empty intersection with the task's countries. - **`application-area`** β€” the file matches if its set contains `all`, or if there is a non-empty intersection with the task's application areas. diff --git a/skills/write.md b/skills/write.md index 6fe2eee..754fe1e 100644 --- a/skills/write.md +++ b/skills/write.md @@ -43,7 +43,7 @@ Knowledge files do not contain code. Samples live as **sibling files** next to t ## Choosing frontmatter values -**`bc-version`.** Default to `[all]` when the guidance is universal β€” a BC language pattern, a property on a long-standing platform type, a CodeCop rule, or a platform behaviour that has not changed across versions. Use an explicit list or range (`[26, 27, 28]`, `[26..28]`) only when the guidance is tied to a version-gated API, a deprecation, or platform behaviour that genuinely differs across versions. Most knowledge files should be `[all]`; reach for a range only with a concrete reason. +**`bc-version`.** Default to `[all]` when the guidance is universal β€” a BC language pattern, a property on a long-standing platform type, a CodeCop rule, or a platform behaviour that has not changed across versions. Use an explicit list or range (`[26, 27, 28]`, `[26..28]`) only when the guidance is tied to a version-gated API, a deprecation, or platform behaviour that genuinely differs across versions. When guidance applies to a feature introduced in version N and not expected to be removed, prefer the open-ended range `[N..]` over a closed range so the file keeps matching future versions β€” reserve a closed upper bound for guidance that genuinely stops applying (for example, a behaviour removed or replaced in a later version). Most knowledge files should be `[all]`; reach for a range only with a concrete reason. **`domain`.** Pick one. If two fit, the file is probably two concerns. If no existing domain fits, introduce a new one β€” domains are open. Prefer existing domains when they are a reasonable fit, to keep retrieval predictable. @@ -65,6 +65,17 @@ Knowledge files do not contain code. Samples live as **sibling files** next to t - **`/community/knowledge//`** β€” shared community patterns. The default layer for contributions from outside the platform team. Content here can be promoted to `/microsoft/` once it proves itself. - **`/custom/knowledge//`** β€” partner or customer overrides. Generally does not appear in the BCQuality repository itself; `/custom/` lives in consumer repositories. +### Writing to `/custom/` β€” fork precondition + +The `/custom/` layer is **empty by default** in the upstream `microsoft/BCQuality` repository β€” it ships as a template (`README.md` plus `.gitkeep` placeholders) and is meant to be populated only inside a **fork or consumer clone** that an organization controls. Custom content is partner- or customer-specific by definition and is never accepted upstream. + +Before authoring or scaffolding any file under `/custom/knowledge/` or `/custom/skills/`, an author β€” human or agent β€” MUST confirm the working repository is **not** `microsoft/BCQuality`: + +- Check the `origin` remote: `git remote get-url origin`. If it points at `github.com/microsoft/BCQuality`, stop β€” you are in the upstream repo, not a fork. +- If you are in the upstream repo, do not write the file. Either fork the repository (or clone it into your organization's own repo) and add the custom content there, or β€” if the guidance is genuinely shareable β€” author it in `/community/knowledge/` instead. + +A pull request that adds `/custom/` content to `microsoft/BCQuality` will be **automatically closed** by the `Guard custom layer` workflow. Validate the fork precondition first so authoring effort is not wasted on a PR that cannot be merged. + ## Pre-PR checklist Before opening a pull request: