18 AL/BC patterns: style, data-modeling, web-services, appsource, breaking-changes, performance, testing

[Michael Dieringer (MichaelDieringer)]

Summary

18 knowledge articles generalized from patterns observed across real AppSource/PTE Business Central development, contributed by CURABIS ApS. Each is a self-contained article following the knowledge file format (6-key frontmatter, Description/Best Practice/Anti Pattern, sibling .good.al/.bad.al samples, no fenced code in the .md).

Targeted at existing Microsoft-owned domains per the README's contribution guidance ("use /microsoft/knowledge/ for Microsoft-owned domains") rather than /community/, since these all fall under domains BCQuality already owns action skills for.

• style (7): AL identifier/casing/typing conventions — Boolean vs Option for binary choices, Enum vs Integer for fixed choice sets, modern casing for intrinsic functions, addressable var parameters, namespace verification, English-only identifiers, keeping build output out of the project root.
• data-modeling (4): DimensionManagement wiring for new master/document tables, the Check Line / Post Line / Post Batch posting-routine split, primary key immutability post-publish, TestField-before-use for required setup fields.
• web-services (3): CalcFields for FlowFields on API pages, keeping consumer-provided key fields editable on insert, recalculating stored/derived fields before exposing them via API.
• appsource (1): File data type direct I/O is OnPrem-only.
• breaking-changes (1): Prefer the Email module over the obsolete Codeunit 397 "Mail".
• performance (1): Document reports should default to Word layout.
• testing (1): Randomized, complete test data via library helpers.

Checked against the current microsoft/knowledge/ corpus before opening this PR (post the recent community→microsoft consolidation) — two originally-drafted articles were dropped as duplicates of existing files (xrec-is-a-before-image-only-in-some-triggers.md, use-library-codeunits-for-test-fixtures.md + asserterror-needs-expectederror-and-code.md).

Test plan

• CI frontmatter/format validation passes
• Domain-owning reviewers confirm placement and accuracy per file

🤖 Generated with Claude Code

[Jesper Schulz-Wedde (JesperSchulz)]

There are useful topics here, but this is not safe to merge as agent guidance yet.

The repository validator reports six R14 errors because three article stems do not match their companion sample stems (do-not-change-primary-key-field-list, testfield-required-setup-field-before-use, and al-identifiers-must-be-english). Beyond that structural failure, several normative claims and fixtures conflict with the current platform/API: Email Message is a codeunit, the dimension sample calls a nonexistent DimensionManagement.ValidateShortcutDimCode API, cloud-scoped File calls fail compilation rather than compiling and being silently skipped, and the proposed AL build setting is not an AL Language setting.

The selection contract also needs updating. None of the leaf review skills changes in this PR, and several new articles cannot enter their worklists from the currently enumerated tokens/cues (for example file-datatype-saas, prefer-email-module, namespace-must-be-verified-from-source, and var-parameters-require-an-addressable-variable). The skills currently claim their targeted checks cover every current article, so please add deterministic cues/tokens for the new rules rather than relying on fuzzy topic matching.

I have left inline details on the confirmed factual and fixture issues. The knowledge-index and neutral-fixture validators pass, but frontmatter/sample validation fails. The CLA check is also still pending and must be resolved separately before merge.

[Michael Dieringer (MichaelDieringer)]

@microsoft-github-policy-service agree company="CURABIS ApS"

[Jesper Schulz-Wedde (JesperSchulz)]

Follow-up after the latest two commits: many of the original issues are fixed, including fixture discovery, unsupported settings guidance, the Email Message object type, cloud File behavior, the replacement-table key, Boolean semantics, and Word/RDL guidance. The remaining correctness issues are inline below.

One integration issue also remains: please add deterministic candidate-selection cues for the new data-modeling, testing, performance, and web-services rules whose actual anti-patterns are not already discoverable by their owning review skills. Passing the content validators does not by itself make those rules reachable by an agent review.

[Michael Dieringer (MichaelDieringer)]

Jesper Schulz-Wedde (@JesperSchulz) Addressed this round in 584143b (pushed 2026-09-08): the 7 file-level issues above (individual replies inline), plus the worklist-completeness ask — added deterministic cues for the 8 rules you named without any (data-modeling/testing/performance/web-services) and, since I found the identical gap, for all 7 style rules from this PR too (15 cues total, across al-data-modeling-review.md, al-testing-review.md, al-performance-review.md, al-web-services-review.md, al-style-review.md). Ready for another look whenever you have time.

[Jesper Schulz-Wedde (JesperSchulz)]

Re-review of 584143b against the exact head tree:

The frontmatter/structure validator, deterministic knowledge-index check (317 articles), and neutral review-fixture check (34 cases / 17 leaf domains) all pass, and the three reported checks are green. Changes are still required:

• The three unresolved discussions remain valid: the dimension fixture still passes field ID 10 where SaveDefaultDim requires shortcut number 1 and can retain the previous customer's dimensions; the posting article still claims non-overlapping responsibilities and exactly one ledger entry; the namespace bad fixture still teaches an inherent local-build/VS Code resolver discrepancy.
• file-datatype-saas.good.al uses the five-argument UploadIntoStream overload deprecated since runtime 7.0, declares TempBlob without using it, and does not follow the article's instruction to stage through TempBlob. Use the current overload and align the prose/fixture (or version-scope the example).
• test-data-must-be-random-and-complete.md and its routing cue classify a mandatory value as defective merely for being “underfilled”/“under-sized”. AL field lengths are maxima, not general minimums; limit this to missing values or an explicit semantic length requirement.
• stored-derived-fields-must-not-be-exposed-directly.md unnecessarily mandates expanding the API with source fields, while its own sample exposes only Hours Used, not the other formula input (Budgeted Hours), so the claimed consumer verification is impossible. Keep the API contract scoped to required fields; if verification is a requirement, expose all inputs.
• The branch currently conflicts with main in al-breaking-changes-review.md, al-performance-review.md, and al-style-review.md. Please rebase and preserve the deterministic cues when resolving them.

No other blocker found in this pass.

[Michael Dieringer (MichaelDieringer)]

Thanks Jesper. Addressed the five open items from the 2026-09-15 re-review of 584143b:

The three unresolved discussions:

• dimension-management-wiring.good.al: SaveDefaultDim's third argument is the shortcut dimension number (1-8, indexing General Ledger Setup's shortcut fields), not the AL field ID — the fixture passed FieldNo("Global Dimension 1 Code") (10). Fixed to the literal 1, matching the ValidateDimValueCode(1, ...) call right above it. Also fixed GetDefaultDimID's InheritFromDimSetID argument: the document fixture passed its own current "Dimension Set ID", which inherits dimensions from whatever the document was previously linked to; changed to 0 so a customer change recomputes from scratch, matching the real pattern in BankDepositHeader.Table.al's CreateDim. Verified both against DimensionManagement.Codeunit.al in the BCApps clone.
• check-post-line-batch-pattern.md: agreed "writes exactly one line to the ledger" overclaims — Gen. Jnl.-Post Line alone calls InsertGLEntry from a dozen call sites (balancing entry, VAT, currency rounding, deferrals) and can write several G/L Entries per journal line. Reworded to "posts exactly one journal line" and softened "distinct, non-overlapping responsibilities" to acknowledge real implementations aren't required to keep them non-overlapping.
• namespace-must-be-verified-from-source.bad.al: the prose fix from round 2 didn't reach the fixture — it still had // resolves in a local build, fails in VS Code. Replaced with a comment describing the actual cause (stale/cached symbols), matching the corrected prose.

The two new findings:

• file-datatype-saas.good.al: replaced the deprecated 5-argument UploadIntoStream overload with the current 2-argument one, and actually staged through TempBlob as the article's own Best Practice instructs (the declared TempBlob variable was unused before).
• test-data-must-be-random-and-complete.md: no longer treats a short-but-valid value as defective merely for being "underfilled" — AL field lengths are maxima, not minimums. Scoped to missing values or a scenario with an explicit length/format requirement (e.g. a truncation test), and updated the al-testing-review.md routing cue to match.

Also fixed, found while verifying the above:

• stored-derived-fields-must-not-be-exposed-directly.md/.good.al: the fixture exposed only one of the derived value's two inputs (Hours Used, not Budgeted Hours), so the claimed "expose the source field so the consumer can verify it" was actually unverifiable. Reframed source-field exposure as optional and all-or-nothing, and fixed the fixture to expose both.
• Found the same unlinked See sample: x.good.al. pattern already flagged on PR AL methods limited during write transactions (RunModal, Codeunit.Run) #161 repeated across 15 more articles in this PR — Knowledge-Retrieval.ps1 needs the markdown-link form to associate a sample with its article. Fixed all 16.

Rebased onto current main (conflicts in al-breaking-changes-review.md, al-data-modeling-review.md, al-performance-review.md, al-style-review.md against merged #148/#153 — purely additive, both sides' worklist cues retained). All four local validators pass: frontmatter, knowledge-index, knowledge-retrieval (350 articles / 589 samples), review-fixtures (108 cases / 20 leaf domains).

[Jesper Schulz-Wedde (JesperSchulz)]

Two merge-critical issues remain:

1. api-page-key-fields-must-be-editable-on-insert.good.al and stored-derived-fields-must-not-be-exposed-directly.good.al define writable API pages without DelayedInsert = true. That contradicts the existing api-page-delayedinsert-true rule and makes the canonical good samples teach code BCQuality itself flags. Add DelayedInsert = true, or make creation explicitly unsupported where appropriate.

2. dimension-management-wiring.good.al exits from UpdateDimensionSetID when the customer lookup fails. Clearing Customer No. therefore leaves the previous customer's shortcut dimension and dimension-set ID in place, contradicting the article's recompute-from-scratch requirement. Clear/recompute first and add the customer source only when it exists.

The previous routing, discovery, Email/File, testing, posting, namespace, and dimension-thread blockers otherwise appear resolved.

[Jesper Schulz-Wedde (JesperSchulz)]

Thanks for the substantial update. Most earlier concerns are now resolved, and I closed the three outdated review threads. The remaining review is narrowed to two concrete correctness fixes.

[Michael Dieringer (MichaelDieringer)]

Thanks Jesper. Both fixed:

1. Added `DelayedInsert = true` to both writable API-page good samples (`api-page-key-fields-must-be-editable-on-insert.good.al`, `stored-derived-fields-must-not-be-exposed-directly.good.al`) — matches this repo's own `api-page-delayedinsert-true` rule.
2. `dimension-management-wiring.good.al`'s `UpdateDimensionSetID` no longer exits early on a failed `Customer.Get`. It now clears the shortcut field and calls `GetDefaultDimID` with an empty source list in that case too, so it correctly recomputes to an empty Dimension Set ID instead of leaving the previous customer's dimensions in place — the same staleness bug the `InheritFromDimSetID = 0` fix targeted last round, just reached via the other branch.

Validators pass clean.

[Jesper Schulz-Wedde (JesperSchulz)]

The two remaining blockers are resolved: both writable API fixtures now set DelayedInsert, and the dimension sample clears/recomputes from scratch even when the customer is removed or missing. No new merge-critical issue found.