mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
Merge branch 'microsoft:main' into main
This commit is contained in:
Michael Dieringer
GitHub
commit
6097fe2c60
121 changed files with 4014 additions and 32 deletions
31
.github/custom-layer-autoclose.md
vendored
Normal file
31
.github/custom-layer-autoclose.md
vendored
Normal file
|
|
@ -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.
|
||||
|
||||
<details>
|
||||
<summary>Files in this PR that triggered the auto-close</summary>
|
||||
|
||||
{{FILES}}
|
||||
</details>
|
||||
|
||||
May your merges be conflict-free. 🚀
|
||||
|
||||
---
|
||||
<sub>🤖 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.</sub>
|
||||
10
.github/new-top-level-flag.md
vendored
Normal file
10
.github/new-top-level-flag.md
vendored
Normal file
|
|
@ -0,0 +1,10 @@
|
|||
<!-- guard:new-top-level -->
|
||||
👋 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.
|
||||
59
.github/scripts/validate_frontmatter.py
vendored
59
.github/scripts/validate_frontmatter.py
vendored
|
|
@ -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
|
||||
|
||||
|
||||
|
|
|
|||
108
.github/workflows/flag-new-top-level.yml
vendored
Normal file
108
.github/workflows/flag-new-top-level.yml
vendored
Normal file
|
|
@ -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 = '<!-- guard:new-top-level -->';
|
||||
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}.`);
|
||||
88
.github/workflows/guard-custom-layer.yml
vendored
Normal file
88
.github/workflows/guard-custom-layer.yml
vendored
Normal file
|
|
@ -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}.`);
|
||||
15
CODEOWNERS
15
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
|
||||
|
|
|
|||
10
README.md
10
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 | ...
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
{
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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)
|
||||
{
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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]) { }
|
||||
}
|
||||
}
|
||||
|
|
@ -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';
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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<Name>.
|
||||
OnBeforePostSalesLine(SalesLine);
|
||||
|
||||
LineAmount := SalesLine.Quantity * SalesLine."Unit Price";
|
||||
// Middle of the routine: On<Name>OnAfter<Context>.
|
||||
OnPostSalesLineOnAfterCalcAmounts(SalesLine, LineAmount);
|
||||
|
||||
SalesLine."Line Amount" := LineAmount;
|
||||
SalesLine.Modify(true);
|
||||
|
||||
// End of the routine: OnAfter<Name>.
|
||||
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<Field>.
|
||||
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<Action>.
|
||||
procedure HandleReportLineProcessing(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
OnBeforeReportPrintLine(SalesLine);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeReportPrintLine(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -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<Name>`; one at the very end is `OnAfter<Name>`; one in the middle names both the host routine and the local boundary, as `On<Name>OnBefore<Context>` or `On<Name>OnAfter<Context>`. 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<Routine>` or `On<Routine>OnBefore`/`OnAfter<Context>` patterns.
|
||||
|
||||
See sample: `name-events-by-publisher-position.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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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 "<concrete impl>"` 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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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 = <Interface> = <Codeunit>;` 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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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 = <Interface> = <Codeunit>;` 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 <Interface>` 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`.
|
||||
|
|
@ -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';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Add table
Add a link
Reference in a new issue