mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
18 more AL/BC patterns: data-modeling, testing, style, security, error-handling, ui, upgrade, web-services, appsource (#157)
* Add 18 more community AL/BC patterns across appsource, data-modeling, error-handling, security, style, testing, ui, upgrade, and web-services Second contribution from CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Cross-checked against the current microsoft/knowledge corpus before opening; several originally-drafted candidates were dropped as duplicates of existing files. * Address Jesper Schulz-Wedde's review on PR #157 - release-must-update-app-version.md: reframe around AppSource's actual strict full-version-ordering requirement; scope branching-policy claims as team convention, not platform rule. - pictures-must-use-media-not-blob.md: MediaSet is a collection of independent media objects, not automatic image variants/thumbnails. - log-writes-must-survive-rollback.{md,good.al}: StartSession's only data channel into the new session is its Record parameter to a TableNo-scoped codeunit; a setter called on a local instance before starting the session populates nothing in the new session. - exposed-objects-must-be-in-a-permission-set.md: correct the three exposure mechanisms (Web Services config, PageType/QueryType=API, ServiceEnabled as a method-only attribute). - pages-must-not-contain-business-logic.md: scope to persisted mutations and cross-entry-point rules; presentation-only calculations and table-owned invariants are not violations. - given-blocks-must-cover-full-precondition-chain.good.al: replace invented LibrarySales calls with the real API (CreateCustomer/CreateSalesOrderForCustomerNo/PostSalesDocument). - test-feature-scenario-tags.{md,good.al}: move [SCENARIO] inside the test procedure body to match the current BCApps corpus; keep [FEATURE] at codeunit level per Microsoft's own documented option. - ui-test-codeunit-naming.md: scope the _UT suffix and adjacent-ID pairing as an explicit team convention, not a BCApps-wide standard. - page-design-must-match-bc-page-type-conventions.md / table-design-must-match-bc-table-type-conventions.md: Card's single-key primary-key claim is a contextual heuristic, not a mandatory constraint (Ship-to Address, Customer/Vendor Bank Account are real composite-key Card pages); a Subsidiary table with its own identity commonly gets List+Card, not Worksheet/Tabular. - api-page-least-privilege-write-access.{md,good.al}: only page-placed fields are ever exposed; set InsertAllowed/DeleteAllowed=false in the good sample so a narrow field set can't still create/delete records. - source-organized-by-feature-not-object-type.md, test-one-when-per-test.md: scope as team/testing-design conventions, not Microsoft platform requirements. - upgrade-tag-logic-must-not-nest-deeply.md: add the Microsoft Learn citation that already backs the two-level nesting limit. - Wire the new articles into the testing/data-modeling/error-handling/ security/ui review skills' candidate-selection signals. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address second round of Jesper Schulz-Wedde's review on PR #157 - log-writes-must-survive-rollback.good.al: fixed invalid trigger OnRun(var Rec: ...) declaration; Rec is implicit when TableNo is set. - exposed-objects-must-be-in-a-permission-set.md: distinguished the three exposure mechanisms (page/query web service or API, codeunit published as a web service, [ServiceEnabled] bound action on a page) and their actual permission targets (page/query "..." = X vs codeunit "..." = X). - code-must-not-change-workdate.md: scoped from an absolute "never" to "not as a side effect of unrelated logic" - verified real WorkDate(x) setter usage in BCApps demo-data generators and test codeunits. - bcpt-scenarios-must-be-app-specific.md: SingleInstance and StartScenario/EndScenario reframed as context-dependent patterns, not mandatory requirements - BCPT Create Customer uses neither. - test-feature-scenario-tags.good.al/.bad.al: replaced the invented LibrarySales.CreateCustomerWithPrice/"Item Price Mgt." calls with a real, verified price-list-line test using Library - Sales/Library - Inventory/ Library - Price Calculation. - page-design-must-match-bc-page-type-conventions.md: scoped the missing UsageCategory anti-pattern to pages intended as searchable entry points. - defensive-vs-offensive-code-must-match-blast-radius.md/.good.al/.bad.al: replaced the VAT registration number "low blast radius" example with a genuinely cosmetic field (customer home page URL). - source-organized-by-feature-not-object-type.md: anti-pattern reframed as inconsistency with a repo's own convention, not the object-type scheme itself. - pictures-must-use-media-not-blob.md: removed leftover "image variants" wording contradicting the already-corrected MediaSet description. Proactively fixed while sweeping all fixtures for invented APIs: - given-blocks-must-cover-full-precondition-chain.bad.al: PostSalesOrder called with wrong arity and referenced an undeclared variable. - ui-test-codeunit-naming.good.al/.bad.al: replaced the same fake "Item Price Mgt."/TestPage "Item Price" with real Library - Sales calls and the real Customer Card TestPage. Worklist completeness: added review-skill cues for the 12 of 18 new rules that had none (al-appsource-review.md, al-data-modeling-review.md, al-error-handling-review.md, al-security-review.md, al-style-review.md x3, al-testing-review.md x2, al-ui-review.md, al-upgrade-review.md, al-web-services-review.md), and fixed test-feature-scenario-tags' cue, which only matched the compliant (tagged) shape instead of the anti-pattern (untagged/generic-named test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix ten focused correctness items plus sample links from Jesper's 2026-09-15 re-review Six carried-over threads: - api-page-least-privilege-write-access fixtures: added the mandatory EntityName/EntitySetName properties (AL0485). - pages-must-not-contain-business-logic fixtures: Sales Line has no "Total Amount" field; replaced with the real "Line Amount" (field 103). - test-feature-scenario-tags.good.al and test-one-when-per-test.good.al: CreatePriceHeader leaves a price list in Draft status, which price calculation ignores. Added Validate(Status, Active) + Modify before the sales line that depends on it. Verified Status field/enum against PriceListHeader.Table.al and PriceStatus.Enum.al in the BCApps clone. - exposed-objects-must-be-in-a-permission-set.md: a published codeunit is a SOAP endpoint (SOAP is deprecated), not OData - Page/Query are the OData object types. Corrected and pointed new integrations at API pages/queries instead. - al-error-handling-review.md: the log-writes-must-survive-rollback cue selected on Session.StartSession, which only appears in the compliant fix, never in the anti-pattern - the bad fixture could never be worklisted. Recued on the actual risk shape (log insert around a failed TryFunction/GetLastError* path, then raise/propagate), with StartSession as an explicit compliant discriminator instead. - page-design-must-match-bc-page-type-conventions.md: the enum value is NavigatePage, not Navigate; noted the type list is a selected subset, not an exhaustive PageType catalogue (PromptDialog, ConfigurationDialog, UserControlHost, XmlPort also exist, out of this article's scope). Four new correctness gaps: - release-must-update-app-version.md: "the version is the only identity" was backwards - id is the app's stable identity, version identifies a release/code-state of it. - defensive-vs-offensive-code-must-match-blast-radius.good.al: the "low blast radius" example had no else branch, so a failed Customer.Get() left the field at its prior/default value instead of the explicit chosen fallback the article claims to demonstrate. Added the else. - bcpt-scenarios-must-be-app-specific.good.al: InitTest and both measured StartScenario/EndScenario sections were empty/comment-only, so the "app-specific" fixture measured no actual work. Filled in a real, self-contained header+line creation path. - upgrade-tag-logic-must-not-nest-deeply.good.al: the flattened version dropped both safety conditions the bad fixture had (Discount % = 0, nonblank posting group), silently changing behavior instead of just removing nesting. Extracted the guarded update into a helper with both conditions preserved as early exits. Also converted this PR's remaining plain-backtick "See sample:" sample references (16 articles) to the READ-convention markdown-link form, matching the fix already made on #156/#158. Rebased onto upstream/main (conflicts in al-ui-review.md, al-style-review.md, al-upgrade-review.md against merged upstream PRs - all additive, both sides' worklist cues retained). * Fix four merge-critical issues from Jesper's 2026-09-22 review - pages-must-not-contain-business-logic.good.al/.bad.al: the "good" codeunit still directly assigned real Sales Line."Line Amount" and called Modify(), bypassing the field's normal Validate cascade (discount, VAT, related-amount maintenance) - persisting inconsistent document lines regardless of which object the code lived in. Replaced the real Sales Line example with a self-contained "Sample Order Line" table and switched the codeunit to Validate()/Modify(true), so the fixture demonstrates the page-vs-codeunit separation without teaching unsafe direct field writes to a real BC document table. - bcpt-scenarios-must-be-app-specific.good.al: Customer.FindFirst() assumed a pre-existing customer (fails against an empty environment), and a session-local NextNo counter for the header key collides across concurrent BCPT sessions and repeated runs. Creates its own customer when none exists, and generates keys from CreateGuid() instead of an in-memory counter. - upgrade-tag-logic-must-not-nest-deeply: the rule conflated two different things - nesting one tag's existence check inside another (the real anti-pattern Microsoft's guidance warns against) with having business-data safety conditions inside a single tagged migration's own loop body (which Microsoft's own worked example does, and its own design guidance explicitly requires: "Implement extra safety checks to avoid data corruption, even though you're using upgrade tags"). Rewrote the Description/Best Practice/Anti Pattern to scope the rule to actual tag nesting and migrations blended under one tag, and rewrote both fixtures: good.al now shows two safety conditions correctly nested inside one migration's own loop plus a second, genuinely separate migration as its own flat tagged procedure; bad.al now shows the real anti-pattern, one tag's check nested inside another's guarded body. - table-design-must-match-bc-table-type-conventions: the rule and its worklist cue fired on any new table with a keys block, forcing buffers, queues, logs, mapping tables, and staging tables into the nearest-looking one of nine business-record archetypes. Added an explicit scope note that these nine types aren't an exhaustive table catalogue, and narrowed the al-data-modeling-review.md cue to require positive evidence (a type-specific naming suffix, key shape, or usage) before worklisting, instead of a bare keys/primary-key declaration. * Narrow upgrade-tag nesting cue to match revised article Cue now flags only nested upgrade-tag checks or functionally unrelated migrations under one tag, and explicitly excludes record loops and business-data safety guards belonging to a single migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Michael Dieringer
GitHub
parent
4287233f80
commit
f63943dcfd
59 changed files with 1612 additions and 2 deletions
|
|
@ -0,0 +1,11 @@
|
|||
// Both fields guarded the same way, out of habit rather than analysis.
|
||||
if Customer.Get(SalesHeader."Sell-to Customer No.") then
|
||||
CustomerHomePage := Customer."Home Page"; // low blast radius - fine
|
||||
|
||||
// but the same pattern, unexamined, was also applied here:
|
||||
if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then
|
||||
VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group"
|
||||
else
|
||||
VATBusPostingGroup := '';
|
||||
// High blast radius: silently wrong VAT posting group reaches posting
|
||||
// with no error, no TestField, and no reviewer in the loop.
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
// Low blast radius: guard, with an explicit chosen fallback.
|
||||
if Customer.Get(SalesHeader."Sell-to Customer No.") then
|
||||
CustomerHomePage := Customer."Home Page"
|
||||
else
|
||||
CustomerHomePage := '';
|
||||
// Blank is an acceptable, deliberately-considered default here - the field
|
||||
// is purely a display convenience and a reviewer sees it before the document ships.
|
||||
// It is assigned explicitly, though, not left to whatever the variable
|
||||
// happened to hold before this lookup ran.
|
||||
|
||||
// High blast radius: let it fail loud, because this feeds posted VAT.
|
||||
SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo);
|
||||
SalesHeader.TestField("VAT Bus. Posting Group");
|
||||
VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group";
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: error-handling
|
||||
keywords: [defensive-programming, offensive-programming, fail-fast, blast-radius, guarded-lookup]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Match defensive vs. offensive error handling to the blast radius of being wrong
|
||||
|
||||
## Description
|
||||
|
||||
Whether code should guard gracefully (defensive) or fail loudly (offensive/fail-fast) is not a matter of habit or a blanket house style — it depends on what happens downstream if the guarded condition is silently defaulted or skipped. Treating every missing value the same way, defensively or offensively, is itself the anti-pattern: uniform defensiveness hides the failures that matter most, while uniform fail-fast turns ordinary, expected absence into unnecessary crashes. Two fields can look structurally identical — both read from a related record, both potentially missing — and still deserve opposite treatment depending on what they feed.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Trace what a silently-defaulted or skipped value actually reaches before deciding how to guard it. If it reaches a posted ledger amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output, code offensively: let the lookup fail loud (`TestField`, an unguarded `Get()` expected to always succeed, or an explicit `Error`) so a human sees the problem before anything posts. If it is cosmetic, informational, or easily corrected after the fact (a display field, an optional UI enhancement, a report not yet run), code defensively — but the fallback must be an explicit, deliberately-chosen, named business value, never a blank or zero that is merely the datatype default. When genuinely unsure which category a field falls into, that is a question to resolve explicitly with whoever owns the requirement, not a coin flip.
|
||||
|
||||
See sample: [`defensive-vs-offensive-code-must-match-blast-radius.good.al`](defensive-vs-offensive-code-must-match-blast-radius.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Guarding two fields the same way purely out of habit, without analyzing what each one feeds. A low-blast-radius field, such as a customer's home page URL shown only for convenience on a printed document, and a high-blast-radius field, such as the VAT posting group that determines VAT actually applied to a posted transaction, are both wrapped in the same `if Header.Get(...) then ... else` pattern with a blank/zero fallback — leaving the posting-critical field free to post with a silently wrong value. A VAT registration number is not a safe stand-in for the low-risk side of this example: it is legally relevant, often validated, and can feed external VAT services or mandated document output, so it belongs on the offensive/fail-fast side alongside the posting group, not next to it as the "safe" contrast.
|
||||
|
||||
See sample: [`defensive-vs-offensive-code-must-match-blast-radius.bad.al`](defensive-vs-offensive-code-must-match-blast-radius.bad.al).
|
||||
|
|
@ -0,0 +1,24 @@
|
|||
codeunit 50101 "Sample Web Service Caller"
|
||||
{
|
||||
procedure CallExternalService()
|
||||
var
|
||||
ErrorLogEntry: Record "Sample Error Log";
|
||||
begin
|
||||
// BUG: the log write happens inside the same transaction as the
|
||||
// risky call, using the same Record instance as the caller.
|
||||
if not TryCallService() then begin
|
||||
ErrorLogEntry.Init();
|
||||
ErrorLogEntry."Error Message" := CopyStr(GetLastErrorText(), 1, 250);
|
||||
ErrorLogEntry.Insert();
|
||||
Error(GetLastErrorText());
|
||||
// Error() above rolls back this transaction - including the
|
||||
// Insert() just made. The failure is never actually logged.
|
||||
end;
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryCallService()
|
||||
begin
|
||||
// ... external call that may fail ...
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,45 @@
|
|||
table 50100 "Sample Error Log Buffer"
|
||||
{
|
||||
TableType = Temporary;
|
||||
fields
|
||||
{
|
||||
field(1; "Call Duration (ms)"; Integer) { }
|
||||
field(2; "Error Message"; Text[250]) { }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50100 "Sample Error Log Writer"
|
||||
{
|
||||
// TableNo makes OnRun receive the Record that Session.StartSession
|
||||
// passes to the new session. This is the only data channel into that
|
||||
// session — there is no shared memory with the caller's instance.
|
||||
TableNo = "Sample Error Log Buffer";
|
||||
|
||||
trigger OnRun()
|
||||
var
|
||||
ErrorLogEntry: Record "Sample Error Log";
|
||||
begin
|
||||
ErrorLogEntry.Init();
|
||||
ErrorLogEntry."Call Duration (ms)" := Rec."Call Duration (ms)";
|
||||
ErrorLogEntry."Error Message" :=
|
||||
CopyStr(Rec."Error Message", 1, MaxStrLen(ErrorLogEntry."Error Message"));
|
||||
ErrorLogEntry.Insert(true);
|
||||
Commit();
|
||||
end;
|
||||
}
|
||||
|
||||
// Caller side: populate the buffer record, then hand it to StartSession.
|
||||
// The insert-and-commit above happens inside the started session, so it
|
||||
// survives even if the caller's own transaction rolls back afterward.
|
||||
codeunit 50101 "Sample Error Log Caller Excerpt"
|
||||
{
|
||||
procedure LogFailure(Duration: Integer; ErrorText: Text)
|
||||
var
|
||||
LogBuffer: Record "Sample Error Log Buffer" temporary;
|
||||
SessionId: Integer;
|
||||
begin
|
||||
LogBuffer."Call Duration (ms)" := Duration;
|
||||
LogBuffer."Error Message" := CopyStr(ErrorText, 1, MaxStrLen(LogBuffer."Error Message"));
|
||||
Session.StartSession(SessionId, Codeunit::"Sample Error Log Writer", CompanyName, LogBuffer);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: error-handling
|
||||
keywords: [logging, rollback, session, transaction, isolated-session, telemetry]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Log writes that must capture failures must survive transaction rollback
|
||||
|
||||
## Description
|
||||
|
||||
Inserting a log record inside the same transaction as the operation it logs looks correct until the operation errors: the transaction rolls back and takes the log entry with it. The result is a log that faithfully records every success and silently loses exactly the failures it exists to capture. This is a common blind spot in error/duration logging around web-service calls, background jobs, and other operations expected to fail sometimes.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`Session.StartSession` on a `TableNo`-scoped codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. `StartSession`'s only channel for getting data into that new session is its optional `Record` parameter, delivered to the target codeunit's `OnRun` trigger — a separate session gets a fresh instantiation of the codeunit, so calling a setter procedure on a local object variable before starting the session does not populate anything in the new session's instance. Capture duration and other telemetry values in the caller, place them into the `Record` passed to `StartSession`, and do the insert-and-commit entirely inside that session's own `OnRun`. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically.
|
||||
|
||||
See sample: [`log-writes-must-survive-rollback.good.al`](log-writes-must-survive-rollback.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Inserting the error-log record in the same transaction as the risky operation, so a rollback deletes the very entry meant to explain the failure. Adding a stray `Commit` before the risky call is not a fix either — it breaks the caller's atomicity and can violate posting-routine rules. Swallowing the error to keep the log alive (running a codeunit without checking or re-raising its result) is equally wrong: the log must observe the failure, not suppress it.
|
||||
|
||||
See sample: [`log-writes-must-survive-rollback.bad.al`](log-writes-must-survive-rollback.bad.al).
|
||||
Loading…
Add table
Add a link
Reference in a new issue