diff --git a/.github/scripts/validate_frontmatter.py b/.github/scripts/validate_frontmatter.py new file mode 100644 index 0000000..df579e2 --- /dev/null +++ b/.github/scripts/validate_frontmatter.py @@ -0,0 +1,577 @@ +#!/usr/bin/env python3 +""" +BCQuality content validator. + +Validates frontmatter, sections, and structural rules for knowledge files, +action skills, meta-skills, and the entry-point skill. Rules derived from +/skills/read.md, /skills/write.md, /skills/do.md, and /skills/entry.md. + +Usage: + python .github/scripts/validate_frontmatter.py [--root PATH] + +Exit status: 0 on success (no errors), 1 on any error. Warnings do not fail. +""" +from __future__ import annotations + +import argparse +import os +import re +import sys +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Iterable + +try: + import yaml +except ImportError: + sys.stderr.write("ERROR: PyYAML is required. Install with: pip install pyyaml\n") + sys.exit(2) + + +# --- Schema constants ------------------------------------------------------- + +KNOWLEDGE_REQUIRED_KEYS = { + "bc-version", "domain", "keywords", "technologies", + "countries", "application-area", +} +ACTION_SKILL_REQUIRED_KEYS = { + "kind", "id", "version", "title", "description", "inputs", "outputs", +} +ACTION_SKILL_OPTIONAL_KEYS = { + "bc-version", "technologies", "countries", "application-area", "sub-skills", +} +META_SKILL_REQUIRED_KEYS = {"kind", "id", "version", "title"} +ENTRY_SKILL_REQUIRED_KEYS = {"kind", "id", "version", "title"} + +STANDARD_INPUTS = { + "pr-diff", "object-list", "file-path", "repository", "telemetry-query", +} +ALLOWED_OUTPUTS = {"findings-report"} +VALID_SAMPLE_KINDS = {"good", "bad"} + +ACTION_SKILL_SECTIONS = ["Source", "Relevance", "Worklist", "Action", "Output"] + +LAYERS = ("microsoft", "community", "custom") +META_SKILL_FILES = {"read.md", "write.md", "do.md"} +ENTRY_SKILL_FILE = "entry.md" + +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+)$") +FENCED_CODE_BLOCK = re.compile(r"^```", re.MULTILINE) +HEADING_H2 = re.compile(r"^##\s+(.+?)\s*$", re.MULTILINE) + + +# --- Diagnostics ------------------------------------------------------------ + +@dataclass +class Diagnostic: + level: str # "error" | "warning" + path: Path + rule: str # e.g. "R03" + message: str + line: int | None = None + + def format_plain(self, root: Path) -> str: + rel = self.path.relative_to(root).as_posix() + prefix = rel if self.line is None else f"{rel}:{self.line}" + return f"{prefix}: [{self.rule}] {self.level}: {self.message}" + + def format_gha(self, root: Path) -> str: + rel = self.path.relative_to(root).as_posix() + loc = f"file={rel}" + if self.line is not None: + loc += f",line={self.line}" + return f"::{self.level} {loc}::[{self.rule}] {self.message}" + + +@dataclass +class Report: + diagnostics: list[Diagnostic] = field(default_factory=list) + + def error(self, path: Path, rule: str, message: str, line: int | None = None) -> None: + self.diagnostics.append(Diagnostic("error", path, rule, message, line)) + + def warn(self, path: Path, rule: str, message: str, line: int | None = None) -> None: + self.diagnostics.append(Diagnostic("warning", path, rule, message, line)) + + @property + def errors(self) -> list[Diagnostic]: + return [d for d in self.diagnostics if d.level == "error"] + + @property + def warnings(self) -> list[Diagnostic]: + return [d for d in self.diagnostics if d.level == "warning"] + + +# --- Frontmatter parsing ---------------------------------------------------- + +@dataclass +class Parsed: + frontmatter: dict[str, Any] | None + body: str + body_start_line: int # 1-based line number where body begins + raw_lines: list[str] + frontmatter_error: str | None # yaml or delimiter issue + + +def parse_markdown(text: str) -> Parsed: + lines = text.splitlines() + if not lines or lines[0].rstrip() != "---": + return Parsed(None, text, 1, lines, "missing opening '---' frontmatter delimiter") + end_idx = None + for i in range(1, len(lines)): + if lines[i].rstrip() == "---": + end_idx = i + break + if end_idx is None: + return Parsed(None, text, 1, lines, "missing closing '---' frontmatter delimiter") + yaml_text = "\n".join(lines[1:end_idx]) + try: + fm = yaml.safe_load(yaml_text) or {} + except yaml.YAMLError as e: + return Parsed(None, text, end_idx + 2, lines, f"YAML parse error: {e}") + if not isinstance(fm, dict): + return Parsed(None, text, end_idx + 2, lines, "frontmatter must be a YAML mapping") + body = "\n".join(lines[end_idx + 1:]) + return Parsed(fm, body, end_idx + 2, lines, None) + + +# --- Small helpers ---------------------------------------------------------- + +def is_non_empty_list_of_str(value: Any) -> bool: + return isinstance(value, list) and len(value) > 0 and all(isinstance(v, str) and v for v in value) + + +def expand_bc_version(value: Any) -> tuple[list[int] | None, str | None]: + """Return (expanded-list, error-message). One of the two is None.""" + if not isinstance(value, list) or not value: + return None, "must be a non-empty list" + # Case 1: all integers + if all(isinstance(v, int) and not isinstance(v, bool) for v in value): + 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]" + 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)) + if start > end: + return None, f"range '{value[0]}' is not ascending" + return list(range(start, end + 1)), None + return None, "must be a list of integers or a single-element range shorthand like [26..28]" + + +def headings_in_order(body: str) -> list[tuple[str, int]]: + """Return list of (heading-text, 1-based line-number-within-body) pairs.""" + out = [] + for i, line in enumerate(body.splitlines(), start=1): + m = re.match(r"^##\s+(.+?)\s*$", line) + if m: + out.append((m.group(1).strip(), i)) + return out + + +# --- Validators ------------------------------------------------------------- + +def validate_knowledge(path: Path, parsed: Parsed, report: Report) -> None: + # R01 frontmatter parseable + if parsed.frontmatter_error: + report.error(path, "R01", parsed.frontmatter_error, 1) + return + fm = parsed.frontmatter + assert fm is not None + + # R02 required keys, no extras, none empty + missing = KNOWLEDGE_REQUIRED_KEYS - fm.keys() + extras = fm.keys() - KNOWLEDGE_REQUIRED_KEYS + if missing: + report.error(path, "R02", f"missing required frontmatter keys: {sorted(missing)}", 1) + if extras: + report.error(path, "R02", f"unexpected frontmatter keys: {sorted(extras)}", 1) + for k in KNOWLEDGE_REQUIRED_KEYS & fm.keys(): + v = fm[k] + if v is None or v == "" or v == []: + report.error(path, "R02", f"frontmatter key '{k}' must not be empty", 1) + + # R03 bc-version + if "bc-version" in fm: + _, err = expand_bc_version(fm["bc-version"]) + if err: + report.error(path, "R03", f"bc-version: {err}", 1) + + # R04 domain + if "domain" in fm: + if not isinstance(fm["domain"], str) or not fm["domain"].strip(): + report.error(path, "R04", "domain must be a non-empty string", 1) + + # R05 keywords + if "keywords" in fm: + kw = fm["keywords"] + if not is_non_empty_list_of_str(kw): + report.error(path, "R05", "keywords must be a non-empty list of strings", 1) + else: + bad = [k for k in kw if not KEBAB_CASE.match(k)] + if bad: + report.error(path, "R05", f"keywords must be lowercase kebab-case: {bad}", 1) + if len(kw) > 10: + report.warn(path, "R05", f"keywords count is {len(kw)}; consider trimming toward ≤10", 1) + + # R06 technologies + if "technologies" in fm: + t = fm["technologies"] + if not is_non_empty_list_of_str(t): + report.error(path, "R06", "technologies must be a non-empty list of strings", 1) + elif "all" in t: + report.error(path, "R06", "technologies must not use the 'all' sentinel; list each technology explicitly", 1) + + # R07 countries + if "countries" in fm: + c = fm["countries"] + if not is_non_empty_list_of_str(c): + report.error(path, "R07", "countries must be a non-empty list of strings", 1) + elif "w1" in c and len(c) > 1: + report.error(path, "R07", "'w1' is mutually exclusive with country codes", 1) + elif "w1" not in c: + bad = [x for x in c if not ISO_ALPHA2.match(x)] + if bad: + report.error(path, "R07", f"countries must be lowercase ISO alpha-2 codes or [w1]: {bad}", 1) + + # R08 application-area + if "application-area" in fm: + a = fm["application-area"] + if not is_non_empty_list_of_str(a): + report.error(path, "R08", "application-area must be a non-empty list of strings", 1) + elif "all" in a and len(a) > 1: + report.error(path, "R08", "'all' is mutually exclusive with specific application areas", 1) + + # R09 has ## Description + headings = [h for h, _ in headings_in_order(parsed.body)] + if "Description" not in headings: + report.error(path, "R09", "missing required '## Description' section") + + # R10 no fenced code blocks + for match in FENCED_CODE_BLOCK.finditer(parsed.body): + # offset to a 1-based line number in the original file + prefix = parsed.body[: match.start()] + body_line = prefix.count("\n") + 1 + file_line = parsed.body_start_line + body_line - 1 + report.error(path, "R10", "knowledge files must not contain fenced code blocks", file_line) + break # one is enough; don't spam + + # R11 file size ≤ 100 lines + total_lines = len(parsed.raw_lines) + if total_lines > MAX_KNOWLEDGE_LINES: + report.error(path, "R11", f"file is {total_lines} lines; max is {MAX_KNOWLEDGE_LINES}") + + +def validate_action_skill(path: Path, parsed: Parsed, report: Report) -> None: + if parsed.frontmatter_error: + report.error(path, "R01", parsed.frontmatter_error, 1) + return + fm = parsed.frontmatter + assert fm is not None + + # R15 required keys; warn on unknown + missing = ACTION_SKILL_REQUIRED_KEYS - fm.keys() + if missing: + report.error(path, "R15", f"missing required action-skill keys: {sorted(missing)}", 1) + unknown = fm.keys() - ACTION_SKILL_REQUIRED_KEYS - ACTION_SKILL_OPTIONAL_KEYS + if unknown: + report.warn(path, "R15", f"unknown action-skill keys: {sorted(unknown)}", 1) + for k in ACTION_SKILL_REQUIRED_KEYS & fm.keys(): + v = fm[k] + if v is None or v == "" or v == []: + report.error(path, "R15", f"action-skill key '{k}' must not be empty", 1) + + # R25 kind matches path + if fm.get("kind") != "action-skill": + report.error(path, "R25", f"file is in a layer skills folder but kind is '{fm.get('kind')}', expected 'action-skill'", 1) + + # R16 id kebab-case, version positive int + if "id" in fm: + if not isinstance(fm["id"], str) or not KEBAB_CASE.match(fm["id"]): + report.error(path, "R16", f"id must be lowercase kebab-case: '{fm['id']}'", 1) + if "version" in fm: + v = fm["version"] + if not isinstance(v, int) or isinstance(v, bool) or v <= 0: + report.error(path, "R16", f"version must be a positive integer: {v!r}", 1) + + # R17 inputs + if "inputs" in fm: + inp = fm["inputs"] + if not is_non_empty_list_of_str(inp): + report.error(path, "R17", "inputs must be a non-empty list of strings", 1) + else: + unknown_inputs = [x for x in inp if x not in STANDARD_INPUTS] + if unknown_inputs: + report.warn(path, "R17", f"inputs contains non-standard values {unknown_inputs}; standard set is {sorted(STANDARD_INPUTS)}", 1) + + # R18 outputs + if "outputs" in fm: + out = fm["outputs"] + if not is_non_empty_list_of_str(out): + report.error(path, "R18", "outputs must be a non-empty list of strings", 1) + else: + bad = [x for x in out if x not in ALLOWED_OUTPUTS] + if bad: + report.error(path, "R18", f"outputs contains non-allowed values {bad}; currently only {sorted(ALLOWED_OUTPUTS)} is defined", 1) + + # R19 optional filter dimensions, if present + if "bc-version" in fm: + _, err = expand_bc_version(fm["bc-version"]) + if err: + report.error(path, "R19", f"bc-version: {err}", 1) + if "technologies" in fm: + t = fm["technologies"] + if not is_non_empty_list_of_str(t): + report.error(path, "R19", "technologies must be a non-empty list of strings", 1) + elif "all" in t: + report.error(path, "R19", "technologies must not use the 'all' sentinel", 1) + if "countries" in fm: + c = fm["countries"] + if not is_non_empty_list_of_str(c): + report.error(path, "R19", "countries must be a non-empty list of strings", 1) + elif "w1" in c and len(c) > 1: + report.error(path, "R19", "'w1' is mutually exclusive with country codes", 1) + elif "w1" not in c: + bad = [x for x in c if not ISO_ALPHA2.match(x)] + if bad: + report.error(path, "R19", f"countries must be ISO alpha-2 or [w1]: {bad}", 1) + if "application-area" in fm: + a = fm["application-area"] + if not is_non_empty_list_of_str(a): + report.error(path, "R19", "application-area must be a non-empty list of strings", 1) + elif "all" in a and len(a) > 1: + report.error(path, "R19", "'all' is mutually exclusive with specific application areas", 1) + + # R20 sub-skills shape + if "sub-skills" in fm: + ss = fm["sub-skills"] + if not is_non_empty_list_of_str(ss): + report.error(path, "R20", "sub-skills must be a non-empty list of repo-relative paths", 1) + else: + bad = [x for x in ss if not x.endswith(".md")] + if bad: + report.error(path, "R20", f"sub-skills entries must end in '.md': {bad}", 1) + + # R21 five required sections, in order, each exactly once + heads = [h for h, _ in headings_in_order(parsed.body)] + indices: list[int] = [] + for required in ACTION_SKILL_SECTIONS: + occurrences = [i for i, h in enumerate(heads) if h == required] + if not occurrences: + report.error(path, "R21", f"missing required section '## {required}'") + elif len(occurrences) > 1: + report.error(path, "R21", f"section '## {required}' appears {len(occurrences)} times; must appear once") + indices.append(occurrences[0]) + else: + indices.append(occurrences[0]) + if len(indices) == len(ACTION_SKILL_SECTIONS) and indices != sorted(indices): + order = [heads[i] for i in indices] + report.error(path, "R21", f"required sections out of order: {order}; expected {ACTION_SKILL_SECTIONS}") + + +def validate_meta_skill(path: Path, parsed: Parsed, report: Report) -> None: + if parsed.frontmatter_error: + report.error(path, "R01", parsed.frontmatter_error, 1) + return + fm = parsed.frontmatter + assert fm is not None + missing = META_SKILL_REQUIRED_KEYS - fm.keys() + if missing: + report.error(path, "R22", f"missing required meta-skill keys: {sorted(missing)}", 1) + for k in META_SKILL_REQUIRED_KEYS & fm.keys(): + v = fm[k] + if v is None or v == "" or v == []: + report.error(path, "R22", f"meta-skill key '{k}' must not be empty", 1) + if fm.get("kind") != "meta-skill": + report.error(path, "R25", f"file in /skills/ is a meta-skill by path but kind is '{fm.get('kind')}', expected 'meta-skill'", 1) + if "id" in fm and (not isinstance(fm["id"], str) or not KEBAB_CASE.match(fm["id"])): + report.error(path, "R22", f"id must be lowercase kebab-case: '{fm['id']}'", 1) + if "version" in fm: + v = fm["version"] + if not isinstance(v, int) or isinstance(v, bool) or v <= 0: + report.error(path, "R22", f"version must be a positive integer: {v!r}", 1) + + +def validate_entry_skill(path: Path, parsed: Parsed, report: Report) -> None: + if parsed.frontmatter_error: + report.error(path, "R01", parsed.frontmatter_error, 1) + return + fm = parsed.frontmatter + assert fm is not None + missing = ENTRY_SKILL_REQUIRED_KEYS - fm.keys() + if missing: + report.error(path, "R23", f"missing required entry-point keys: {sorted(missing)}", 1) + for k in ENTRY_SKILL_REQUIRED_KEYS & fm.keys(): + v = fm[k] + if v is None or v == "" or v == []: + report.error(path, "R23", f"entry-point key '{k}' must not be empty", 1) + if fm.get("kind") != "entry-point": + report.error(path, "R25", f"file is /skills/entry.md but kind is '{fm.get('kind')}', expected 'entry-point'", 1) + if fm.get("id") != "entry": + report.error(path, "R23", f"entry-point id must be 'entry', got '{fm.get('id')}'", 1) + if "version" in fm: + v = fm["version"] + if not isinstance(v, int) or isinstance(v, bool) or v <= 0: + report.error(path, "R23", f"version must be a positive integer: {v!r}", 1) + + +# --- Path and sample checks ------------------------------------------------- + +def classify(path_from_root: Path) -> str | None: + """Return 'knowledge' | 'action-skill' | 'meta' | 'entry' | None.""" + parts = path_from_root.parts + if len(parts) < 2: + return None + top = parts[0] + if top == "skills": + if len(parts) == 2: + name = parts[1] + if name == ENTRY_SKILL_FILE: + return "entry" + if name in META_SKILL_FILES: + return "meta" + return None + if top in LAYERS and path_from_root.suffix == ".md": + if len(parts) >= 3 and parts[1] == "skills": + return "action-skill" + if len(parts) >= 4 and parts[1] == "knowledge": + return "knowledge" + return None + + +def validate_knowledge_path(path: Path, root: Path, report: Report) -> None: + rel = path.relative_to(root) + parts = rel.parts + # R13 expected shape: /knowledge//.md + if len(parts) != 4: + report.error(path, "R13", f"knowledge file must live at /knowledge//.md; got {rel.as_posix()}") + return + slug = path.stem + # R12 filename kebab-case + if not KEBAB_CASE.match(slug): + report.error(path, "R12", f"filename slug must be lowercase kebab-case: '{slug}'") + + +def validate_samples_in_domain(domain_dir: Path, root: Path, report: Report) -> None: + """R14: every non-.md file must match .. with .md present.""" + if not domain_dir.is_dir(): + return + article_slugs = {p.stem for p in domain_dir.glob("*.md")} + for entry in domain_dir.iterdir(): + if not entry.is_file() or entry.suffix == ".md": + continue + name = entry.name + # Expect .. + m = re.match(r"^(?P[a-z0-9]+(?:-[a-z0-9]+)*)\.(?P[a-z0-9]+)\.(?P[a-z0-9]+)$", name) + if not m: + report.error(entry, "R14", f"sample file name must match '..' with kebab-case slug: '{name}'") + continue + slug = m.group("slug") + kind = m.group("kind") + if slug not in article_slugs: + report.error(entry, "R14", f"orphan sample: no matching article '{slug}.md' in {domain_dir.relative_to(root).as_posix()}") + if kind not in VALID_SAMPLE_KINDS: + report.warn(entry, "R14", f"non-standard sample kind '{kind}'; standard kinds are {sorted(VALID_SAMPLE_KINDS)}") + + +# --- Orchestration ---------------------------------------------------------- + +@dataclass +class SkillRecord: + path: Path + kind: str # frontmatter kind + skill_id: str | None + + +def run(root: Path) -> Report: + report = Report() + skill_records: list[SkillRecord] = [] + + # Walk declared top-level folders only; avoid wandering into .git, etc. + walk_roots = [root / "skills"] + [root / layer for layer in LAYERS] + candidate_files: list[Path] = [] + for wr in walk_roots: + if wr.exists(): + candidate_files.extend(p for p in wr.rglob("*") if p.is_file()) + + # First pass: classify and validate each file + for path in candidate_files: + rel = path.relative_to(root) + kind = classify(rel) + if kind is None: + continue + try: + text = path.read_text(encoding="utf-8") + except UnicodeDecodeError as e: + report.error(path, "R01", f"file is not valid UTF-8: {e}") + continue + parsed = parse_markdown(text) + + if kind == "knowledge": + validate_knowledge_path(path, root, report) + validate_knowledge(path, parsed, report) + elif kind == "action-skill": + validate_action_skill(path, parsed, report) + if parsed.frontmatter and isinstance(parsed.frontmatter.get("id"), str): + skill_records.append(SkillRecord(path, "action-skill", parsed.frontmatter["id"])) + elif kind == "meta": + validate_meta_skill(path, parsed, report) + if parsed.frontmatter and isinstance(parsed.frontmatter.get("id"), str): + skill_records.append(SkillRecord(path, "meta-skill", parsed.frontmatter["id"])) + elif kind == "entry": + validate_entry_skill(path, parsed, report) + if parsed.frontmatter and isinstance(parsed.frontmatter.get("id"), str): + skill_records.append(SkillRecord(path, "entry-point", parsed.frontmatter["id"])) + + # Second pass: sample files per knowledge domain + for layer in LAYERS: + kn_root = root / layer / "knowledge" + if not kn_root.is_dir(): + continue + for domain_dir in kn_root.iterdir(): + if domain_dir.is_dir(): + validate_samples_in_domain(domain_dir, root, report) + + # Third pass: R24 unique ids within kind + by_kind: dict[str, dict[str, list[Path]]] = {} + for rec in skill_records: + if rec.skill_id is None: + continue + by_kind.setdefault(rec.kind, {}).setdefault(rec.skill_id, []).append(rec.path) + for kind, by_id in by_kind.items(): + for sid, paths in by_id.items(): + if len(paths) > 1: + for p in paths: + 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}") + + return report + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="BCQuality frontmatter and structure validator.") + parser.add_argument("--root", default=".", help="Repository root (default: current directory).") + args = parser.parse_args(argv) + + root = Path(args.root).resolve() + report = run(root) + + gha = os.environ.get("GITHUB_ACTIONS") == "true" + for d in report.diagnostics: + line = d.format_gha(root) if gha else d.format_plain(root) + print(line) + + n_err = len(report.errors) + n_warn = len(report.warnings) + print(f"\nValidator: {n_err} error(s), {n_warn} warning(s)") + return 1 if n_err else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/validate-frontmatter.yml b/.github/workflows/validate-frontmatter.yml new file mode 100644 index 0000000..1a66239 --- /dev/null +++ b/.github/workflows/validate-frontmatter.yml @@ -0,0 +1,25 @@ +name: Validate frontmatter and structure + +on: + pull_request: + branches: [main] + push: + branches: [main] + +jobs: + validate: + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install dependencies + run: pip install pyyaml + + - name: Run validator + run: python .github/scripts/validate_frontmatter.py --root . diff --git a/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.bad.al b/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.bad.al new file mode 100644 index 0000000..c499e27 --- /dev/null +++ b/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.bad.al @@ -0,0 +1,16 @@ +codeunit 50100 "Event Audit Buffer" +{ + SingleInstance = true; + + // Unbounded global: every event fires adds an entry for the lifetime of the session. + var + AllEventIds: List of [Guid]; + + [EventSubscriber(ObjectType::Table, Database::"Sales Header", OnAfterInsertEvent, '', false, false)] + local procedure OnAfterInsertSalesHeader(var Rec: Record "Sales Header") + begin + // No cap. No eviction. No reset. A session that sees ten thousand inserts + // keeps ten thousand GUIDs in memory until the user signs out. + AllEventIds.Add(Rec.SystemId); + end; +} diff --git a/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.good.al b/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.good.al new file mode 100644 index 0000000..bc93073 --- /dev/null +++ b/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.good.al @@ -0,0 +1,28 @@ +codeunit 50100 "Event Audit Buffer" +{ + SingleInstance = true; + + var + RecentEventIds: List of [Guid]; + MaxBuffered: Integer; + + trigger OnRun() + begin + MaxBuffered := 50; + end; + + [EventSubscriber(ObjectType::Table, Database::"Sales Header", OnAfterInsertEvent, '', false, false)] + local procedure OnAfterInsertSalesHeader(var Rec: Record "Sales Header") + begin + // Bounded cache: drop the oldest entry when the cap is reached. + RecentEventIds.Add(Rec.SystemId); + if RecentEventIds.Count() > MaxBuffered then + RecentEventIds.RemoveAt(1); + end; + + procedure ResetAtBusinessProcessBoundary() + begin + // Explicit reset point at a natural boundary in the workflow. + Clear(RecentEventIds); + end; +} diff --git a/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.md b/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.md new file mode 100644 index 0000000..d2de14e --- /dev/null +++ b/community/knowledge/performance/avoid-growing-globals-in-singleinstance-subscribers.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [singleinstance, subscriber, event, memory, session] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Avoid growing globals in SingleInstance subscribers + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +A codeunit with `SingleInstance = true` is allocated once per session and lives until the session ends. Global variables on it are never collected between event fires. A subscriber that accumulates data into a global — buffering payloads, appending to a list, caching without a cap — steadily grows its session footprint for the entire user session. The symptom is memory that only recovers on sign-out, and it surfaces only on long-running sessions. + +## Best Practice + +Keep the global footprint on a SingleInstance subscriber bounded and intentional: a handful of flags, a setup record, a bounded cache with a maximum size. When cross-event state is genuinely needed, define an explicit reset point — end of a business process, arrival of a specific terminal event — that clears the growing collection. + +See sample: `avoid-growing-globals-in-singleinstance-subscribers.good.al`. + +## Anti Pattern + +A SingleInstance subscriber that appends each event's payload to a global list, dictionary, or temporary record without a cap or cleanup trigger. The list grows for hours, memory pressure builds quietly, and debugging the root cause on a live environment is substantially harder than noticing the unbounded append in code review. + +See sample: `avoid-growing-globals-in-singleinstance-subscribers.bad.al`. diff --git a/community/knowledge/performance/call-setloadfields-before-filters.bad.al b/community/knowledge/performance/call-setloadfields-before-filters.bad.al new file mode 100644 index 0000000..65b64f1 --- /dev/null +++ b/community/knowledge/performance/call-setloadfields-before-filters.bad.al @@ -0,0 +1,25 @@ +codeunit 50100 "Customer Credit Report" +{ + procedure ReportCreditLimits(StartNo: Code[20]; EndNo: Code[20]) + var + Customer: Record Customer; + begin + // Filters applied first. + Customer.SetRange("No.", StartNo, EndNo); + Customer.SetRange(Blocked, Customer.Blocked::" "); + + // SetLoadFields is too late - the platform has already planned the + // query for the full record. The call is paid for without delivering + // any of the optimization benefit. + Customer.SetLoadFields("No.", Name, "Credit Limit (LCY)"); + + if Customer.FindSet() then + repeat + EmitLine(Customer."No.", Customer.Name, Customer."Credit Limit (LCY)"); + until Customer.Next() = 0; + end; + + local procedure EmitLine(CustNo: Code[20]; Name: Text; CreditLimit: Decimal) + begin + end; +} diff --git a/community/knowledge/performance/call-setloadfields-before-filters.good.al b/community/knowledge/performance/call-setloadfields-before-filters.good.al new file mode 100644 index 0000000..65a1426 --- /dev/null +++ b/community/knowledge/performance/call-setloadfields-before-filters.good.al @@ -0,0 +1,24 @@ +codeunit 50100 "Customer Credit Report" +{ + procedure ReportCreditLimits(StartNo: Code[20]; EndNo: Code[20]) + var + Customer: Record Customer; + begin + // 1. Declare the minimal load first, before any filter. + Customer.SetLoadFields("No.", Name, "Credit Limit (LCY)"); + + // 2. Apply filters. + Customer.SetRange("No.", StartNo, EndNo); + Customer.SetRange(Blocked, Customer.Blocked::" "); + + // 3. Iterate; the query loads only the three declared fields. + if Customer.FindSet() then + repeat + EmitLine(Customer."No.", Customer.Name, Customer."Credit Limit (LCY)"); + until Customer.Next() = 0; + end; + + local procedure EmitLine(CustNo: Code[20]; Name: Text; CreditLimit: Decimal) + begin + end; +} diff --git a/community/knowledge/performance/call-setloadfields-before-filters.md b/community/knowledge/performance/call-setloadfields-before-filters.md new file mode 100644 index 0000000..90d95e8 --- /dev/null +++ b/community/knowledge/performance/call-setloadfields-before-filters.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [setloadfields, placement, filter, setrange, query-plan] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Call SetLoadFields before filters + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +`SetLoadFields` is folded into the database query that the subsequent `Find`, `FindSet`, or `FindFirst` executes. When it is called after filters have already been applied, the platform either ignores the specification or is forced into an extra round-trip to reload the narrower column set — negating the optimization. The placement rule is simple and absolute: `SetLoadFields` must come first. + +## Best Practice + +Use a consistent order on every record variable that participates in `SetLoadFields` optimization: declare the record, call `SetLoadFields` with the processing fields, apply `SetRange`/`SetFilter`, then `FindSet` and iterate. The order makes the optimization visible in code review and prevents accidental regressions when filters are refactored. + +See sample: `call-setloadfields-before-filters.good.al`. + +## Anti Pattern + +Setting filters first — because the filter logic is what the reviewer is thinking about — and then adding `SetLoadFields` just before the `FindSet`. The platform has already planned the query with the full column set; the `SetLoadFields` call is paid for without delivering any of the benefit. + +See sample: `call-setloadfields-before-filters.bad.al`. diff --git a/community/knowledge/performance/choose-maintainsiftindex-by-read-write-ratio.good.al b/community/knowledge/performance/choose-maintainsiftindex-by-read-write-ratio.good.al new file mode 100644 index 0000000..473575a --- /dev/null +++ b/community/knowledge/performance/choose-maintainsiftindex-by-read-write-ratio.good.al @@ -0,0 +1,32 @@ +table 50100 "Item Ledger Entry (Demo)" +{ + fields + { + field(1; "Entry No."; Integer) { DataClassification = SystemMetadata; } + field(2; "Item No."; Code[20]) { DataClassification = CustomerContent; } + field(3; "Posting Date"; Date) { DataClassification = CustomerContent; } + field(4; Quantity; Decimal) { DataClassification = CustomerContent; } + field(5; "Cost Amount"; Decimal) { DataClassification = CustomerContent; } + } + + keys + { + key(PK; "Entry No.") { Clustered = true; } + + // Write-heavy ledger key: aggregates on this key are read rarely relative + // to INSERT frequency. Keeping SIFT live on every write is net-negative. + key(ByItemAndDate; "Item No.", "Posting Date") + { + SumIndexFields = Quantity, "Cost Amount"; + MaintainSIFTIndex = false; + } + + // Dashboard-facing key: aggregates read on every session load, underlying + // rows updated infrequently. Keeping SIFT live pays for itself. + key(ByItem; "Item No.") + { + SumIndexFields = Quantity; + MaintainSIFTIndex = true; + } + } +} diff --git a/community/knowledge/performance/choose-maintainsiftindex-by-read-write-ratio.md b/community/knowledge/performance/choose-maintainsiftindex-by-read-write-ratio.md new file mode 100644 index 0000000..787c344 --- /dev/null +++ b/community/knowledge/performance/choose-maintainsiftindex-by-read-write-ratio.md @@ -0,0 +1,26 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [maintainsiftindex, sift, calcsums, flowfield, write-cost] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Choose MaintainSIFTIndex by read-write ratio + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +`MaintainSIFTIndex` on a key decides whether the SIFT aggregate structure is updated on every `INSERT`, `MODIFY`, and `DELETE` that touches the key's fields. With `Yes`, `CalcSums` and FlowField reads are immediate — but every write pays the cost of updating the aggregate. With `No`, writes are cheaper but the first aggregate read after a change has to rebuild. Neither value is universally correct; the right choice depends on how often the aggregate is read versus how often the underlying rows are written. + +## Best Practice + +Measure read-to-write ratios for the key's SIFT fields under realistic workloads. Set `MaintainSIFTIndex = Yes` only on keys whose aggregates are read far more often than the rows are written (reporting keys on reference tables, dashboards). Set `No` on keys whose rows are written heavily and whose aggregates are read rarely (transactional ledger entries, import-staging tables). + +See sample: `choose-maintainsiftindex-by-read-write-ratio.good.al`. + +## Anti Pattern + +Leaving `MaintainSIFTIndex = Yes` on every key by reflex or convenience. On write-heavy tables the cumulative cost turns every INSERT or MODIFY into several additional aggregate updates, and the impact compounds in batch imports and posting routines — often without any code-review signal that the property is the cause. diff --git a/community/knowledge/performance/load-common-fields-before-branching-on-case.bad.al b/community/knowledge/performance/load-common-fields-before-branching-on-case.bad.al new file mode 100644 index 0000000..22d6de9 --- /dev/null +++ b/community/knowledge/performance/load-common-fields-before-branching-on-case.bad.al @@ -0,0 +1,23 @@ +codeunit 50100 "Sales Document Processor" +{ + procedure ProcessDocument(var SalesHeader: Record "Sales Header") + begin + // Single top-level load pulls every field any branch might touch. + // Order records pay for Posting Date and Amount Including VAT that + // only the Invoice branch reads, and vice versa. + SalesHeader.SetLoadFields( + "Document Type", "No.", "Sell-to Customer No.", + "Order Date", "Shipment Date", "Completely Shipped", + "Posting Date", "Amount Including VAT"); + + case SalesHeader."Document Type" of + SalesHeader."Document Type"::Order: + ProcessOrder(SalesHeader); + SalesHeader."Document Type"::Invoice: + ProcessInvoice(SalesHeader); + end; + end; + + local procedure ProcessOrder(var SalesHeader: Record "Sales Header") begin end; + local procedure ProcessInvoice(var SalesHeader: Record "Sales Header") begin end; +} diff --git a/community/knowledge/performance/load-common-fields-before-branching-on-case.good.al b/community/knowledge/performance/load-common-fields-before-branching-on-case.good.al new file mode 100644 index 0000000..ec70b4c --- /dev/null +++ b/community/knowledge/performance/load-common-fields-before-branching-on-case.good.al @@ -0,0 +1,25 @@ +codeunit 50100 "Sales Document Processor" +{ + procedure ProcessDocument(var SalesHeader: Record "Sales Header") + begin + // Tier 1: the discriminator and any fields every branch reads. + SalesHeader.SetLoadFields("Document Type", "No.", "Sell-to Customer No."); + + case SalesHeader."Document Type" of + SalesHeader."Document Type"::Order: + begin + // Tier 2: extend the load only on the branch that needs these fields. + SalesHeader.SetLoadFields("Order Date", "Shipment Date", "Completely Shipped"); + ProcessOrder(SalesHeader); + end; + SalesHeader."Document Type"::Invoice: + begin + SalesHeader.SetLoadFields("Posting Date", "Amount Including VAT"); + ProcessInvoice(SalesHeader); + end; + end; + end; + + local procedure ProcessOrder(var SalesHeader: Record "Sales Header") begin end; + local procedure ProcessInvoice(var SalesHeader: Record "Sales Header") begin end; +} diff --git a/community/knowledge/performance/load-common-fields-before-branching-on-case.md b/community/knowledge/performance/load-common-fields-before-branching-on-case.md new file mode 100644 index 0000000..1cc8492 --- /dev/null +++ b/community/knowledge/performance/load-common-fields-before-branching-on-case.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [setloadfields, case, conditional, branch, field-loading] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Load common fields before branching on case + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +When record processing branches on state, different branches typically read different fields. A single `SetLoadFields` at the top listing every field any branch might touch pulls more data than any individual execution path needs — on the hot path, the rest is loaded for nothing. A two-tier approach matches loading to actual usage: load the fields the `case` expression evaluates plus any fields every branch uses, then add a branch-local `SetLoadFields` inside each branch for that branch's extra fields. + +## Best Practice + +Before the `case`, call `SetLoadFields` with the minimal set — the discriminator field and fields common to every branch. Inside each branch, before the first access to a branch-specific field, add a second `SetLoadFields` covering those fields. The platform honors the in-branch call for the next record operation, so the extra data is fetched only when the branch runs. + +See sample: `load-common-fields-before-branching-on-case.good.al`. + +## Anti Pattern + +A single top-level `SetLoadFields` enumerating every field any branch might read. On records whose state routes them to the fast common branch, the rarely-needed fields are still loaded — the optimization becomes a net-neutral or net-negative change on the hot path. + +See sample: `load-common-fields-before-branching-on-case.bad.al`. diff --git a/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.bad.al b/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.bad.al new file mode 100644 index 0000000..13d620f --- /dev/null +++ b/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.bad.al @@ -0,0 +1,18 @@ +codeunit 50100 "Item Reindex Queue" +{ + procedure QueueItemsForReindex(CategoryCode: Code[20]) + var + Item: Record Item; + ReindexQueue: Codeunit "Reindex Queue"; + begin + // Default full-record load. Description, Unit Price, Inventory, and + // every other column are fetched across the wire and held in memory + // for the whole loop - the body only ever reads "No.". + Item.SetRange("Item Category Code", CategoryCode); + + if Item.FindSet() then + repeat + ReindexQueue.Enqueue(Item."No."); + until Item.Next() = 0; + end; +} diff --git a/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.good.al b/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.good.al new file mode 100644 index 0000000..5dcc499 --- /dev/null +++ b/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.good.al @@ -0,0 +1,17 @@ +codeunit 50100 "Item Reindex Queue" +{ + procedure QueueItemsForReindex(CategoryCode: Code[20]) + var + Item: Record Item; + ReindexQueue: Codeunit "Reindex Queue"; + begin + // Only the primary key is used in the loop body; load nothing else. + Item.SetLoadFields("No."); + Item.SetRange("Item Category Code", CategoryCode); + + if Item.FindSet() then + repeat + ReindexQueue.Enqueue(Item."No."); + until Item.Next() = 0; + end; +} diff --git a/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.md b/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.md new file mode 100644 index 0000000..f13f444 --- /dev/null +++ b/community/knowledge/performance/load-only-primary-key-fields-for-reference-work.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [setloadfields, primary-key, reference, existence-check, memory] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Load only primary key fields for reference work + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +Work that uses a record only for its identity — passing it to another procedure that will re-fetch what it needs, queueing a key for later processing, running existence checks, or building a reference collection — does not need non-key payload fields. `SetLoadFields` with only the primary key fields loads the minimum that preserves record identity while skipping everything else. On wide tables with large text, BLOB, or media fields the difference in memory and transfer is substantial. + +## Best Practice + +When the iterating code's body touches only primary key fields (or passes the record to another procedure that will apply its own `SetLoadFields`), declare `SetLoadFields` with just the primary key fields before applying filters and calling `FindSet`. Callers downstream that need more fields issue their own `Get` or extend the load explicitly. + +See sample: `load-only-primary-key-fields-for-reference-work.good.al`. + +## Anti Pattern + +Using the default full-record load in loops whose body only reads the primary key, or forwards the record to another codeunit that immediately re-queries. The non-key payload is fetched across the wire and held in memory for the duration of the loop, then discarded unread. + +See sample: `load-only-primary-key-fields-for-reference-work.bad.al`. diff --git a/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.bad.al b/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.bad.al new file mode 100644 index 0000000..66e59af --- /dev/null +++ b/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.bad.al @@ -0,0 +1,24 @@ +codeunit 50100 "Recent Orders Summary" +{ + procedure SummarizeRecentOrders(StartDate: Date; EndDate: Date) + var + SalesHeader: Record "Sales Header"; + begin + // "Document Type" and "Document Date" are listed in SetLoadFields even + // though they appear only in filters. Per-row values are transferred + // for columns the processing body never reads. + SalesHeader.SetLoadFields( + "Document Type", "Document Date", + "No.", "Sell-to Customer No.", "Amount Including VAT"); + + SalesHeader.SetRange("Document Type", SalesHeader."Document Type"::Order); + SalesHeader.SetRange("Document Date", StartDate, EndDate); + + if SalesHeader.FindSet() then + repeat + Emit(SalesHeader."No.", SalesHeader."Sell-to Customer No.", SalesHeader."Amount Including VAT"); + until SalesHeader.Next() = 0; + end; + + local procedure Emit(No: Code[20]; CustNo: Code[20]; Amount: Decimal) begin end; +} diff --git a/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.good.al b/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.good.al new file mode 100644 index 0000000..6e98764 --- /dev/null +++ b/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.good.al @@ -0,0 +1,22 @@ +codeunit 50100 "Recent Orders Summary" +{ + procedure SummarizeRecentOrders(StartDate: Date; EndDate: Date) + var + SalesHeader: Record "Sales Header"; + begin + // "Document Type" and "Document Date" are used only in the filters below. + // The database index handles them; there is no need to load their values + // into AL memory for every row. + SalesHeader.SetLoadFields("No.", "Sell-to Customer No.", "Amount Including VAT"); + + SalesHeader.SetRange("Document Type", SalesHeader."Document Type"::Order); + SalesHeader.SetRange("Document Date", StartDate, EndDate); + + if SalesHeader.FindSet() then + repeat + Emit(SalesHeader."No.", SalesHeader."Sell-to Customer No.", SalesHeader."Amount Including VAT"); + until SalesHeader.Next() = 0; + end; + + local procedure Emit(No: Code[20]; CustNo: Code[20]; Amount: Decimal) begin end; +} diff --git a/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.md b/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.md new file mode 100644 index 0000000..3674836 --- /dev/null +++ b/community/knowledge/performance/omit-filter-only-fields-from-setloadfields.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [setloadfields, filter, field-exclusion, index] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Omit filter-only fields from SetLoadFields + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +Fields used only in `SetRange` and `SetFilter` do their work at the database level using indexes; their values never need to be loaded into AL memory for the filter to apply. Listing such fields in `SetLoadFields` costs the transfer and memory footprint of every row's value for no functional benefit. Distinguishing filter-only fields from processing fields keeps the loaded column set as narrow as the iterating code actually reads. + +## Best Practice + +Include in `SetLoadFields` exactly the fields the iterating code reads. Fields referenced only in `SetRange`/`SetFilter` stay out of the list — filtering continues to work correctly because the database uses the index. Treat the audit as "what does the `repeat…until` block touch?" rather than "what does this procedure mention?". + +See sample: `omit-filter-only-fields-from-setloadfields.good.al`. + +## Anti Pattern + +Listing every field the procedure mentions in `SetLoadFields`, including date-range or status fields that appear only in filters. The loaded record now carries per-row values for columns the processing body never reads, inflating memory and network cost without changing any behavior. + +See sample: `omit-filter-only-fields-from-setloadfields.bad.al`. diff --git a/community/knowledge/performance/order-case-branches-by-frequency.bad.al b/community/knowledge/performance/order-case-branches-by-frequency.bad.al new file mode 100644 index 0000000..ee3a6ec --- /dev/null +++ b/community/knowledge/performance/order-case-branches-by-frequency.bad.al @@ -0,0 +1,26 @@ +codeunit 50100 "Document Router" +{ + procedure Route(SalesHeader: Record "Sales Header") + begin + // Alphabetical ordering. Every Order (the ~85% common case) evaluates + // "Credit Memo", "Invoice", and "Quote" before matching. + case SalesHeader."Document Type" of + SalesHeader."Document Type"::"Credit Memo": + RouteCreditMemo(SalesHeader); + SalesHeader."Document Type"::Invoice: + RouteInvoice(SalesHeader); + SalesHeader."Document Type"::Quote: + RouteQuote(SalesHeader); + SalesHeader."Document Type"::Order: + RouteOrder(SalesHeader); + SalesHeader."Document Type"::"Return Order": + RouteReturnOrder(SalesHeader); + end; + end; + + local procedure RouteOrder(SalesHeader: Record "Sales Header") begin end; + local procedure RouteInvoice(SalesHeader: Record "Sales Header") begin end; + local procedure RouteQuote(SalesHeader: Record "Sales Header") begin end; + local procedure RouteCreditMemo(SalesHeader: Record "Sales Header") begin end; + local procedure RouteReturnOrder(SalesHeader: Record "Sales Header") begin end; +} diff --git a/community/knowledge/performance/order-case-branches-by-frequency.good.al b/community/knowledge/performance/order-case-branches-by-frequency.good.al new file mode 100644 index 0000000..c650375 --- /dev/null +++ b/community/knowledge/performance/order-case-branches-by-frequency.good.al @@ -0,0 +1,25 @@ +codeunit 50100 "Document Router" +{ + procedure Route(SalesHeader: Record "Sales Header") + begin + // In this deployment Orders are ~85% of posting calls, Invoices ~12%, + // and the rest are edge cases. The hot branch goes first. + case SalesHeader."Document Type" of + SalesHeader."Document Type"::Order: + RouteOrder(SalesHeader); + SalesHeader."Document Type"::Invoice: + RouteInvoice(SalesHeader); + SalesHeader."Document Type"::"Credit Memo": + RouteCreditMemo(SalesHeader); + SalesHeader."Document Type"::"Return Order": + RouteReturnOrder(SalesHeader); + else + Error('Unexpected document type %1', SalesHeader."Document Type"); + end; + end; + + local procedure RouteOrder(SalesHeader: Record "Sales Header") begin end; + local procedure RouteInvoice(SalesHeader: Record "Sales Header") begin end; + local procedure RouteCreditMemo(SalesHeader: Record "Sales Header") begin end; + local procedure RouteReturnOrder(SalesHeader: Record "Sales Header") begin end; +} diff --git a/community/knowledge/performance/order-case-branches-by-frequency.md b/community/knowledge/performance/order-case-branches-by-frequency.md new file mode 100644 index 0000000..90518f6 --- /dev/null +++ b/community/knowledge/performance/order-case-branches-by-frequency.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [case, branch, frequency, control-flow, hot-path] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Order case branches by frequency + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +The AL `case` statement evaluates branches in the order they appear. When the distribution of the discriminator is heavily skewed — one or two values handle the vast majority of records, and the rest handle edge cases — the average cost of the statement is dominated by how many branches precede the common one. For evenly distributed discriminators the order does not matter; for skewed distributions it changes the hot-path cost of every call site. + +## Best Practice + +Where the runtime frequency of values is known or measurable, list the common branches first. An `else` arm that handles unexpected values belongs last. When the common branch is also the simplest to evaluate, the placement compounds: the hot path is both short and cheap, and the uncommon branches are never touched on typical records. + +See sample: `order-case-branches-by-frequency.good.al`. + +## Anti Pattern + +Ordering branches alphabetically, by enum declaration order, or by "logical grouping" when the runtime distribution is heavily skewed. Every common record pays the cost of evaluating every uncommon branch first; on a posting routine processing thousands of rows the overhead is measurable. + +See sample: `order-case-branches-by-frequency.bad.al`. diff --git a/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.bad.al b/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.bad.al new file mode 100644 index 0000000..9e016d1 --- /dev/null +++ b/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.bad.al @@ -0,0 +1,19 @@ +codeunit 50100 "Stale Quote Cleanup" +{ + procedure ClearExpiredQuotes(CutoffDate: Date) + var + SalesHeader: Record "Sales Header"; + begin + SalesHeader.SetRange("Document Type", SalesHeader."Document Type"::Quote); + SalesHeader.SetFilter("Document Date", '<%1', CutoffDate); + SalesHeader.SetRange(Status, SalesHeader.Status::Open); + + // One SQL DELETE per row. On a 10k-row cleanup, minutes instead of + // under a second - and the OnDelete trigger has no logic this call + // needs to run. + if SalesHeader.FindSet() then + repeat + SalesHeader.Delete(); + until SalesHeader.Next() = 0; + end; +} diff --git a/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.good.al b/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.good.al new file mode 100644 index 0000000..697edd9 --- /dev/null +++ b/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.good.al @@ -0,0 +1,17 @@ +codeunit 50100 "Stale Quote Cleanup" +{ + procedure ClearExpiredQuotes(CutoffDate: Date) + var + SalesHeader: Record "Sales Header"; + begin + // OnDelete on Sales Header carries no logic this call depends on: + // expired quotes have no ledger entries, shipments, or downstream state. + SalesHeader.SetRange("Document Type", SalesHeader."Document Type"::Quote); + SalesHeader.SetFilter("Document Date", '<%1', CutoffDate); + SalesHeader.SetRange(Status, SalesHeader.Status::Open); + + // Single SQL DELETE. Orders of magnitude faster than FindSet + Delete + // once the filtered set exceeds a handful of rows. + SalesHeader.DeleteAll(); + end; +} diff --git a/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md b/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md new file mode 100644 index 0000000..194dae4 --- /dev/null +++ b/community/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: performance +keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use DeleteAll for filtered bulk deletion + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +`DeleteAll` translates to a single SQL `DELETE` with the record variable's current filters applied as the WHERE clause. A loop of `FindSet` + `Delete` instead issues one SQL statement per row. On any dataset larger than a handful of records, the gap is an order of magnitude or more. The tradeoff is that `DeleteAll` bypasses the `OnDelete` table trigger, so the decision hinges on whether that trigger's logic is required for this specific deletion. + +## Best Practice + +After narrowing the record set with `SetRange`/`SetFilter`, use `DeleteAll` whenever the `OnDelete` trigger has no logic that this call depends on — typically the case for housekeeping routines, staging-table cleanup, and deletions already validated upstream. When the trigger IS required, either keep the explicit loop-plus-`Delete` pattern and comment why, or pre-run the trigger logic against a temporary buffer and then `DeleteAll` the primary table. + +See sample: `use-deleteall-for-filtered-bulk-deletion.good.al`. + +## Anti Pattern + +Iterating with `FindSet` + `Delete` to clear a filtered set of records that carry no meaningful `OnDelete` logic. Every row pays a full AL round-trip; on a ten-thousand-row cleanup the loop can take minutes where `DeleteAll` takes under a second. + +See sample: `use-deleteall-for-filtered-bulk-deletion.bad.al`. diff --git a/community/knowledge/security/classify-every-field-with-dataclassification.bad.al b/community/knowledge/security/classify-every-field-with-dataclassification.bad.al new file mode 100644 index 0000000..a9ae935 --- /dev/null +++ b/community/knowledge/security/classify-every-field-with-dataclassification.bad.al @@ -0,0 +1,28 @@ +table 50100 "Customer Feedback" +{ + fields + { + field(1; "Feedback No."; Code[20]) + { + // No DataClassification declared. Defaults to ToBeClassified. + } + field(2; "Contact Name"; Text[100]) + { + DataClassification = ToBeClassified; + } + field(3; "Email"; Text[80]) + { + // Personal data classified as CustomerContent understates privacy impact. + DataClassification = CustomerContent; + } + field(4; "Feedback Text"; Text[2048]) + { + DataClassification = ToBeClassified; + } + } + + keys + { + key(PK; "Feedback No.") { Clustered = true; } + } +} diff --git a/community/knowledge/security/classify-every-field-with-dataclassification.good.al b/community/knowledge/security/classify-every-field-with-dataclassification.good.al new file mode 100644 index 0000000..baa3079 --- /dev/null +++ b/community/knowledge/security/classify-every-field-with-dataclassification.good.al @@ -0,0 +1,36 @@ +table 50100 "Customer Feedback" +{ + fields + { + field(1; "Feedback No."; Code[20]) + { + DataClassification = SystemMetadata; + } + field(2; "Contact Name"; Text[100]) + { + DataClassification = EndUserIdentifiableInformation; + } + field(3; "Email"; Text[80]) + { + DataClassification = EndUserIdentifiableInformation; + } + field(4; "Product Code"; Code[20]) + { + DataClassification = CustomerContent; + } + field(5; "Feedback Text"; Text[2048]) + { + // When uncertain between CustomerContent and EUII, prefer the stronger protection. + DataClassification = EndUserIdentifiableInformation; + } + field(6; "Submitted DateTime"; DateTime) + { + DataClassification = SystemMetadata; + } + } + + keys + { + key(PK; "Feedback No.") { Clustered = true; } + } +} diff --git a/community/knowledge/security/classify-every-field-with-dataclassification.md b/community/knowledge/security/classify-every-field-with-dataclassification.md new file mode 100644 index 0000000..540f64b --- /dev/null +++ b/community/knowledge/security/classify-every-field-with-dataclassification.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: security +keywords: [dataclassification, gdpr, privacy, euii, compliance] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Classify every field with DataClassification + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +Every field on every AL table and table extension must carry an explicit `DataClassification` property. The value drives GDPR tooling, data-subject requests, retention policies, and audit reporting — all of which rely on the field metadata to know what data to include, anonymize, or delete. A field with no `DataClassification` defaults to `ToBeClassified`, which is a compliance gap, not a neutral state. + +## Best Practice + +Choose the narrowest value that accurately describes the field's content: `EndUserIdentifiableInformation` for data that directly identifies a person, `EndUserPseudonymousIdentifiers` for indirect identifiers, `CustomerContent` for business operational data, `SystemMetadata` for system-generated housekeeping, `AccountData` for tenant/billing, `OrganizationIdentifiableInformation` for organization-level identifiers. When uncertain between two values, pick the stronger protection. + +See sample: `classify-every-field-with-dataclassification.good.al`. + +## Anti Pattern + +Leaving `DataClassification = ToBeClassified` on a field, or omitting the property entirely (which resolves to the same default). Code in this state fails compliance audits and breaks the subject-access-request and retention tooling that depends on the property being set correctly. + +See sample: `classify-every-field-with-dataclassification.bad.al`. diff --git a/community/knowledge/security/compose-permission-sets-with-included-sets.bad.al b/community/knowledge/security/compose-permission-sets-with-included-sets.bad.al new file mode 100644 index 0000000..f8ae6f4 --- /dev/null +++ b/community/knowledge/security/compose-permission-sets-with-included-sets.bad.al @@ -0,0 +1,21 @@ +// Two role-shaped sets, each re-enumerating the same objects. Adding a new +// Sales table means editing both sets by hand; forgetting one creates a +// subtle authorization bug where one role was updated and its sibling was not. + +permissionset 50110 "Sales Order Processor" +{ + Assignable = true; + Permissions = + tabledata Customer = IM, + tabledata "Sales Header" = IMD, + tabledata "Sales Line" = IMD; +} + +permissionset 50111 "Sales Viewer" +{ + Assignable = true; + Permissions = + tabledata Customer = R, + tabledata "Sales Header" = R, + tabledata "Sales Line" = R; +} diff --git a/community/knowledge/security/compose-permission-sets-with-included-sets.good.al b/community/knowledge/security/compose-permission-sets-with-included-sets.good.al new file mode 100644 index 0000000..52f6fa0 --- /dev/null +++ b/community/knowledge/security/compose-permission-sets-with-included-sets.good.al @@ -0,0 +1,34 @@ +// Building blocks: focused per-concern, marked Assignable = false so administrators +// do not accidentally assign a fragment. +permissionset 50100 "Sales Tables - Read" +{ + Assignable = false; + Permissions = + tabledata Customer = R, + tabledata "Sales Header" = R, + tabledata "Sales Line" = R; +} + +permissionset 50101 "Sales Tables - Edit" +{ + Assignable = false; + IncludedPermissionSets = "Sales Tables - Read"; + Permissions = + tabledata Customer = IM, + tabledata "Sales Header" = IMD, + tabledata "Sales Line" = IMD; +} + +// Role-shaped, Assignable = true, composed from building blocks. +// Adding a new Sales table means editing one building block; both roles inherit the change. +permissionset 50110 "Sales Order Processor" +{ + Assignable = true; + IncludedPermissionSets = "Sales Tables - Edit"; +} + +permissionset 50111 "Sales Viewer" +{ + Assignable = true; + IncludedPermissionSets = "Sales Tables - Read"; +} diff --git a/community/knowledge/security/compose-permission-sets-with-included-sets.md b/community/knowledge/security/compose-permission-sets-with-included-sets.md new file mode 100644 index 0000000..3de55d0 --- /dev/null +++ b/community/knowledge/security/compose-permission-sets-with-included-sets.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: security +keywords: [permissionset, includedpermissionsets, assignable, composition, role] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Compose permission sets with IncludedPermissionSets + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +The `IncludedPermissionSets` property lets one AL permission set reference another, composing rights out of smaller building blocks. Combined with `Assignable = false` on the building blocks, an extension can ship focused per-module units (a table-data cluster, an API-access cluster) and assemble role-shaped sets that include them. Adding an object updates one building block, and every role-shaped set that includes it inherits the change automatically — instead of drifting apart across duplicated definitions. + +## Best Practice + +Break permission grants into small, focused building blocks, one per cohesive concern. Mark the building blocks `Assignable = false` so administrators do not accidentally assign a fragment. Build role-shaped, `Assignable = true` sets that reference the relevant building blocks through `IncludedPermissionSets`. When the extension grows, the structure absorbs the growth without duplicated edits. + +See sample: `compose-permission-sets-with-included-sets.good.al`. + +## Anti Pattern + +Declaring several role-shaped permission sets that each re-enumerate the same object lists. Adding a new table means touching every set by hand; the sets drift apart over time, and subtle authorization bugs appear where one role was updated and a sibling role was not. + +See sample: `compose-permission-sets-with-included-sets.bad.al`. diff --git a/community/knowledge/security/do-not-grant-rights-beyond-a-users-entitlement.md b/community/knowledge/security/do-not-grant-rights-beyond-a-users-entitlement.md new file mode 100644 index 0000000..dfc2666 --- /dev/null +++ b/community/knowledge/security/do-not-grant-rights-beyond-a-users-entitlement.md @@ -0,0 +1,26 @@ +--- +bc-version: [26..28] +domain: security +keywords: [entitlement, permissionset, license, clipping, sandbox-drift] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not grant rights beyond a user's entitlement + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +Entitlements are license-level caps on what a user can access, derived automatically from the BC license tier. Permission sets are application-level grants administered on top of the entitlement. A permission set can only grant within the entitlement's boundaries; grants beyond those boundaries are silently clipped at runtime. This means a permission set authored and validated in a developer sandbox (with a broad license) can appear to work correctly there and fail silently in a customer tenant where users hold a narrower entitlement. + +## Best Practice + +When designing a permission set that ships with an extension, consult the entitlement model for the target user population before finalizing the grants. Every object and tabledata right the set expects to grant should be reachable within the intended entitlement tier; if it is not, the set needs to be scoped to licenses that permit it, or the feature needs a different access path. + +See sample: `do-not-grant-rights-beyond-a-users-entitlement.good.al`. + +## Anti Pattern + +Authoring permission sets in a sandbox with full-license context and shipping them without verifying which entitlement tier customer users actually hold. The sets look complete in test; on a real customer they silently lose rights at runtime and the symptom is "the feature does not work for some users" with no obvious authorization error. diff --git a/community/knowledge/security/guard-bulk-operations-with-istemporary.bad.al b/community/knowledge/security/guard-bulk-operations-with-istemporary.bad.al new file mode 100644 index 0000000..f906eed --- /dev/null +++ b/community/knowledge/security/guard-bulk-operations-with-istemporary.bad.al @@ -0,0 +1,10 @@ +codeunit 50100 "Order Buffer Helper" +{ + procedure ResetStagingBuffer(var OrderBuffer: Record "Sales Header") + begin + // No IsTemporary check. A caller that accidentally passes the real + // Sales Header table wipes every sales header in the company with + // no prior warning. + OrderBuffer.DeleteAll(); + end; +} diff --git a/community/knowledge/security/guard-bulk-operations-with-istemporary.good.al b/community/knowledge/security/guard-bulk-operations-with-istemporary.good.al new file mode 100644 index 0000000..b597c53 --- /dev/null +++ b/community/knowledge/security/guard-bulk-operations-with-istemporary.good.al @@ -0,0 +1,12 @@ +codeunit 50100 "Order Buffer Helper" +{ + procedure ResetStagingBuffer(var OrderBuffer: Record "Sales Header") + begin + // The helper is designed for a temporary buffer only. Fail loudly + // if a caller accidentally passes the real table. + if not OrderBuffer.IsTemporary() then + Error('ResetStagingBuffer requires a temporary Sales Header; a persistent record was passed.'); + + OrderBuffer.DeleteAll(); + end; +} diff --git a/community/knowledge/security/guard-bulk-operations-with-istemporary.md b/community/knowledge/security/guard-bulk-operations-with-istemporary.md new file mode 100644 index 0000000..baf2bd8 --- /dev/null +++ b/community/knowledge/security/guard-bulk-operations-with-istemporary.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: security +keywords: [istemporary, deleteall, modifyall, safeguard, precondition] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Guard bulk operations with IsTemporary + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +An AL helper that accepts a `var Rec: Record X` parameter and performs a bulk operation (`DeleteAll`, `ModifyAll`, or an unfiltered loop that mutates every record) cannot tell from the signature alone whether the caller passed a temporary buffer or the real table. A misuse that passes the real table wipes or rewrites live data at production scale with no earlier warning. A single `IsTemporary` check at the procedure entry turns a silent-corruption risk into an early, actionable failure. + +## Best Practice + +Any helper designed to operate on a temporary record, and that performs `DeleteAll`, `ModifyAll`, or similar bulk writes on its parameter, should call `Rec.IsTemporary()` at the top and raise a descriptive error when the assumption is violated. The error message should name the parameter so the misuse is easy to locate. + +See sample: `guard-bulk-operations-with-istemporary.good.al`. + +## Anti Pattern + +Trusting documentation or naming conventions alone to signal that a `var Rec` parameter is expected to be temporary. A future refactor or a copy-paste caller can pass the real table; the bulk operation then executes against production rows silently. + +See sample: `guard-bulk-operations-with-istemporary.bad.al`. diff --git a/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.bad.al b/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.bad.al new file mode 100644 index 0000000..e294f38 --- /dev/null +++ b/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.bad.al @@ -0,0 +1,29 @@ +codeunit 50100 "Partner API Client" +{ + procedure FetchOrders(var Response: Text): Boolean + var + HttpClient: HttpClient; + HttpRequest: HttpRequestMessage; + HttpResponse: HttpResponseMessage; + ApiKey: Text; + begin + // API key stored as plain Text in a setup table - not IsolatedStorage, + // not SecretText. Rotation means the admin editing a Text field; + // a single disclosure exposes every tenant running this extension. + ApiKey := GetApiKeyFromSetupTable(); + + HttpRequest.SetRequestUri('https://partner.example.com/orders'); + HttpRequest.Method('GET'); + HttpRequest.GetHeaders().Add('X-API-Key', ApiKey); + + if not HttpClient.Send(HttpRequest, HttpResponse) then + exit(false); + + HttpResponse.Content.ReadAs(Response); + exit(HttpResponse.IsSuccessStatusCode); + end; + + local procedure GetApiKeyFromSetupTable(): Text + begin + end; +} diff --git a/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.good.al b/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.good.al new file mode 100644 index 0000000..18a066f --- /dev/null +++ b/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.good.al @@ -0,0 +1,44 @@ +codeunit 50100 "Partner API Client" +{ + procedure FetchOrders(var Response: Text): Boolean + var + OAuth2: Codeunit OAuth2; + HttpClient: HttpClient; + HttpRequest: HttpRequestMessage; + HttpResponse: HttpResponseMessage; + AccessToken: SecretText; + Scopes: List of [Text]; + begin + Scopes.Add('https://partner.example.com/.default'); + + // Client-credentials flow for service-to-service. Tokens expire and rotate + // on their own schedule; secret and client id are retrieved from IsolatedStorage. + if not OAuth2.AcquireTokenWithClientCredentials( + GetClientIdFromIsolatedStorage(), + GetClientSecretFromIsolatedStorage(), + 'https://login.example.com/tenantid/oauth2/v2.0/token', + '', + Scopes, + AccessToken) + then + exit(false); + + HttpRequest.SetRequestUri('https://partner.example.com/orders'); + HttpRequest.Method('GET'); + HttpRequest.GetHeaders().Add('Authorization', SecretStrSubstNo('Bearer %1', AccessToken)); + + if not HttpClient.Send(HttpRequest, HttpResponse) then + exit(false); + + HttpResponse.Content.ReadAs(Response); + exit(HttpResponse.IsSuccessStatusCode); + end; + + local procedure GetClientIdFromIsolatedStorage(): Text + begin + end; + + local procedure GetClientSecretFromIsolatedStorage(): SecretText + begin + end; +} diff --git a/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.md b/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.md new file mode 100644 index 0000000..ab30675 --- /dev/null +++ b/community/knowledge/security/prefer-oauth2-over-api-keys-for-external-http-calls.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: security +keywords: [oauth2, api-key, authentication, httpclient, token-refresh] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Prefer OAuth2 over API keys for external HTTP calls + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +External HTTP integrations from AL can authenticate using OAuth 2.0 (client-credentials for service-to-service, authorization-code for user-delegated), API keys, basic authentication, or credentials in URLs. The mechanisms differ substantially in the blast radius of a leaked secret and in how cleanly tokens can be rotated. OAuth-issued tokens expire on their own schedule and rotate cleanly; API keys and basic-auth passwords typically have to be rotated manually and usually live unencrypted in a configuration table. When the partner supports OAuth, the difference is a material security improvement, not a stylistic preference. + +## Best Practice + +When the partner supports OAuth, use the platform `OAuth2` codeunit (`AcquireTokenWithClientCredentials` for service-to-service, `AcquireAuthorizationCodeTokenFromCache` for user-delegated flows) rather than hand-rolled token acquisition. Carry tokens and client secrets as `SecretText`, persist them only in IsolatedStorage, and refresh tokens proactively — on a buffer before the documented expiry — so routine calls never block on a token refresh. + +See sample: `prefer-oauth2-over-api-keys-for-external-http-calls.good.al`. + +## Anti Pattern + +Accepting an API-key or basic-auth integration because it is the first option documented, even when the partner supports OAuth. The shared secret usually ends up in a setup-table `Text` field, rotation becomes a manual operation that rarely happens, and a single disclosure exposes every tenant using the extension. + +See sample: `prefer-oauth2-over-api-keys-for-external-http-calls.bad.al`. diff --git a/community/knowledge/security/protect-sensitive-data-in-temporary-tables.bad.al b/community/knowledge/security/protect-sensitive-data-in-temporary-tables.bad.al new file mode 100644 index 0000000..3a0a0db --- /dev/null +++ b/community/knowledge/security/protect-sensitive-data-in-temporary-tables.bad.al @@ -0,0 +1,27 @@ +codeunit 50100 "Customer Temp Processor" +{ + // Global temporary buffer - survives across procedure calls, carries values + // to unrelated callers that may have no right to see them. + var + GlobalTempCustomer: Record Customer temporary; + + procedure LoadCustomersForExport(FilterText: Text) + var + Customer: Record Customer; + begin + // No ReadPermission check before populating. + Customer.SetFilter("No.", FilterText); + if Customer.FindSet() then + repeat + GlobalTempCustomer := Customer; + GlobalTempCustomer.Insert(); + until Customer.Next() = 0; + + ExportBuffer(); + // No DeleteAll. Data remains in the global for the lifetime of the codeunit. + end; + + local procedure ExportBuffer() + begin + end; +} diff --git a/community/knowledge/security/protect-sensitive-data-in-temporary-tables.good.al b/community/knowledge/security/protect-sensitive-data-in-temporary-tables.good.al new file mode 100644 index 0000000..54bf2bb --- /dev/null +++ b/community/knowledge/security/protect-sensitive-data-in-temporary-tables.good.al @@ -0,0 +1,32 @@ +codeunit 50100 "Customer Temp Processor" +{ + procedure BuildScopedCustomerBuffer(CustomerNoFilter: Text): Boolean + var + Customer: Record Customer; + TempCustomer: Record Customer temporary; + begin + // Validate the caller's permission before copying sensitive rows. + if not Customer.ReadPermission() then + exit(false); + + Customer.SetFilter("No.", CustomerNoFilter); + if not Customer.FindSet() then + exit(true); + + repeat + TempCustomer := Customer; + TempCustomer.Insert(); + until Customer.Next() = 0; + + ProcessCustomerBuffer(TempCustomer); + + // Explicit cleanup on the normal exit path. + TempCustomer.DeleteAll(); + exit(true); + end; + + local procedure ProcessCustomerBuffer(var TempCustomer: Record Customer temporary) + begin + // Use the buffer in-place; do not persist values elsewhere. + end; +} diff --git a/community/knowledge/security/protect-sensitive-data-in-temporary-tables.md b/community/knowledge/security/protect-sensitive-data-in-temporary-tables.md new file mode 100644 index 0000000..e6d475d --- /dev/null +++ b/community/knowledge/security/protect-sensitive-data-in-temporary-tables.md @@ -0,0 +1,28 @@ +--- +bc-version: [26..28] +domain: security +keywords: [temporary-table, data-protection, permission, cleanup] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Protect sensitive data in temporary tables + +> **Seed article.** Ported from BC Code Intelligence to seed the community corpus. Community contributors are invited to expand or refine. + +## Description + +A temporary record copies data out of the source table into session memory. The platform does not automatically enforce the source table's permission model on the copy, and a value written to a temporary buffer can outlive the procedure that put it there if the buffer is a global or is passed upward. Code that places sensitive rows into a temporary table is therefore responsible for the checks and cleanup the source table would otherwise provide. + +## Best Practice + +Validate the caller's read permission on the source table before populating the temporary buffer. Keep the buffer's lifetime as short as the work requires, and delete its contents on every exit path — including error paths — so sensitive values do not linger. Prefer local temporary variables over globals for anything carrying sensitive data. + +See sample: `protect-sensitive-data-in-temporary-tables.good.al`. + +## Anti Pattern + +Copying records into a temporary buffer without a preceding permission check, and relying on procedure-exit to clean up. An exception before the explicit cleanup leaves the data in the buffer; a global or var-parameter buffer carries the data back to callers that may have no right to see it. + +See sample: `protect-sensitive-data-in-temporary-tables.bad.al`.