mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
0a8c9a8556
* Add 18 community AL/BC patterns across style, data-modeling, web-services, appsource, breaking-changes, performance, and testing Contributed by CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Each article follows the knowledge file format (frontmatter, Description/Best Practice/Anti Pattern, sibling .good.al/.bad.al samples). * Address Jesper Schulz-Wedde's review on PR #156 - Rename 3 articles so their .good.al/.bad.al companion stems match (do-not-change-primary-key, testfield-required-setup-field, al-identifiers-english), fixing the R14 orphan-sample errors. - do-not-change-primary-key.good.al: include Flow in the new table's own primary key so it actually models the discriminating dimension. - al-build-output-must-not-pollute-project-root.md: drop the unsubstantiated AL0197 causal claim and the non-existent al.outputPath setting; reframe as build-artifact hygiene sourced from ALTool --outfolder / al_build outputPath. - prefer-email-module.md: Email Message is Codeunit 8904, not a table; distinguish it from the underlying Sent/Outbox/Draft storage. - file-datatype-saas.md: File.Open/Create/Read/Write fails to compile against a Cloud-scoped project, it does not compile and silently fail at runtime. - namespace-must-be-verified-from-source.md: narrow to "resolve from the referenced object's source or symbols," since source-file line one is not the only authoritative source (symbol packages, comments before the namespace line). - test-data-must-be-random-and-complete.md: drop "assume an empty database" and "collision-free" absolutes; reframe around independence from unrelated business records and reserving explicit values for scenario-defining inputs. - binary-choice-must-be-boolean.md: scope to genuine true/false semantics, not mechanical two-member-enum-to-boolean conversion. - document-report-word-layout.md: scope down to a sourced Microsoft Learn recommendation instead of an unconditional performance guarantee; cite the three Learn pages. - Wire the new articles into their review skills' candidate-selection signals (file-datatype-saas, prefer-email-module, namespace-must-be-verified-from-source, var-parameters-require-an- addressable-variable) so they can actually enter a worklist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix dimension-management-wiring.md: ValidateShortcutDimCode and CreateDim do not exist on the current DimensionManagement codeunit Verified against microsoft/BCApps: the real master-table validation procedure is ValidateDimValueCode (or ValidateShortcutDimValues when a DimSetID is also needed), and the real document-side inheritance procedure is GetDefaultDimID, not CreateDim. Caught from Jesper Schulz-Wedde's review thread, which had been partially hidden by GitHub's comment folding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address second round of Jesper Schulz-Wedde's review on PR #156 - dimension-management-wiring.md/.good.al: split into the two distinct models the article was conflating - master data (Default Dimension records via ValidateDimValueCode/SaveDefaultDim) vs. transactional/ document data (a single Dimension Set ID assembled via AddDimSource + GetDefaultDimID, verified against BCApps' ExchRateAdjmtProcess.Codeunit.al). Added a compiling document-table example alongside the existing master table one. - Deleted api-page-flowfields-must-be-calcfields (.md/.good.al/.bad.al): Microsoft's own FlowFields documentation states a FlowField used as a control's direct source expression is automatically calculated on any page - no API-page exception is documented, and none could be reproduced. - prefer-email-module.bad.al/.md: Codeunit Mail has no Send/GetErrorDesc members; fixed to the real current 7-argument CreateMessage signature, and corrected the claim that the legacy path "still runs" - its base implementation no longer sends anything, only raises integration events. - check-post-line-batch-pattern.md/.good.al: reframed from a universal invariant to the standard shape, naming the real Gen./Item/CA/Res./Job/ Insurance/Mfg. Item/FA Jnl.-Check Line/-Post Line/-Post Batch codeunits it's based on. Added the missing Check Line companion codeunit so the good fixture is internally complete. - test-data-must-be-random-and-complete.good.al: removed leftover "collision-free" wording contradicting the already-corrected article text. - fixed-choice-set-must-use-enum-not-integer.md: removed the reintroduced state-count heuristic ("the line is the state count"), aligned with binary-choice-must-be-boolean.md's semantics-based distinction. - namespace-must-be-verified-from-source.md: removed the false claim that the compiler and AL Language Server use different namespace-resolution rules. - intrinsic-al-functions-must-use-modern-casing.md: removed the unverified claim that PascalCase is the VS Code formatter's default output. Worklist completeness: added cues for the 8 rules in data-modeling, testing, performance, and web-services that had none (Jesper's explicit ask), plus the same gap in all 7 style rules from this PR (not explicitly named this round, but the identical systemic issue) - 15 cues total across al-data-modeling-review.md, al-testing-review.md, al-performance-review.md, al-web-services-review.md, and al-style-review.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix remaining correctness issues from Jesper's 2026-09-15 re-review - dimension-management-wiring: SaveDefaultDim's third argument is the shortcut dimension number (1-8), not the field's AL field ID; the fixture passed FieldNo(...) = 10. GetDefaultDimID's InheritFromDimSetID must be 0 when recomputing after the linking record changes, not the document's existing Dimension Set ID (which would retain the previous customer's leftover dimensions). Verified against DimensionManagement.Codeunit.al and BankDepositHeader.Table.al in the BCApps reference clone. - check-post-line-batch-pattern: "Post Line writes exactly one line to the ledger" overclaimed - 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 the "distinct, non-overlapping responsibilities" absolute. - namespace-must-be-verified-from-source.bad.al: dropped the "resolves in a local build, fails in VS Code" comment (taught an inherent compiler/language-server disagreement that isn't real); reframed as stale/cached symbols, matching the prose fix already made. - 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 previously unused). - test-data-must-be-random-and-complete: 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). Updated the al-testing-review.md routing cue to match. - stored-derived-fields-must-not-be-exposed-directly: stopped mandating source-field exposure as part of the core pattern: the good fixture exposed only one of the derived value's two inputs (Hours Used, not Budgeted Hours), making the claimed "so the consumer can verify it" impossible. Reframed as an optional, all-or-nothing addition and fixed the fixture to expose both inputs. Rebased onto upstream/main to resolve conflicts in al-breaking-changes-review.md, al-data-modeling-review.md, al-performance-review.md, and al-style-review.md against merged PRs #148 and #153; all sides' worklist tokens/cues retained. * Fix remaining READ-convention sample links across this PR's 18 articles The same plain-backtick "See sample: \`x.good.al\`." form fixed on al-methods-limited-during-write-transactions (PR #161) turned up repo-wide on 15 more of this PR's articles - Knowledge-Retrieval.ps1 requires the markdown-link form to associate a sample with its article. All 16 fixed; the four local validators (frontmatter, knowledge-index, knowledge-retrieval, review-fixtures, skill-index) pass. * Fix two merge-critical correctness issues from Jesper's 2026-09-22 review - api-page-key-fields-must-be-editable-on-insert.good.al and stored-derived-fields-must-not-be-exposed-directly.good.al: both were writable API pages missing DelayedInsert = true, contradicting this repo's own api-page-delayedinsert-true rule - the canonical "good" samples were teaching code BCQuality itself flags. - dimension-management-wiring.good.al: UpdateDimensionSetID exited early when Customer.Get failed, leaving the previous customer's shortcut dimension and Dimension Set ID in place - the same staleness bug the InheritFromDimSetID = 0 fix (from the prior review round) was meant to prevent, just triggered by a failed lookup instead of a successful one. Now clears the shortcut field and recomputes with an empty source list on a failed lookup too, so GetDefaultDimID correctly returns an empty Dimension Set ID instead of never running. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2.1 KiB
2.1 KiB
| bc-version | domain | keywords | technologies | countries | application-area | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
data-modeling |
|
|
|
|
Never Change a Published Table's Primary or Clustered Key Field List
Contributions welcome — open a PR to refine or extend this article.
Description
Once a table has shipped — to AppSource, or to any customer environment that has already upgraded onto it — its primary key, and any other key marked Clustered = true, is frozen. This includes adding a field to the key, not only removing or reordering one: Business Central identifies existing rows by their key value, so any change to which fields compose that key invalidates every row already stored under the old shape, and the platform's upgrade validation rejects it outright (AS0009). This is easy to trip over because it doesn't look like the well-known "don't delete a field" mistake — the field being added is often brand new, and folding a new discriminating dimension straight into the existing key feels like the natural, un-denormalized way to model it. On an unpublished table that is correct; on a published one it is a breaking schema change regardless of which direction the field list changed, and there is no in-place fix once the upgrade is rejected, only reverting the key to its published shape.
Best Practice
Leave a published table's key exactly as shipped. Model a new discriminating dimension as a separate table with its own key instead of adding a field to the existing key, and branch orchestration code by the new dimension rather than filtering one shared table on an extra key field.
See sample: do-not-change-primary-key.good.al.
Anti Pattern
Adding a field to a published table's primary or clustered key to distinguish a new case. This fails AppSource validation or any customer upgrade with AS0009 as soon as rows already exist under the old key shape, whether the field is being added, removed, or reordered.
See sample: do-not-change-primary-key.bad.al.