AL methods limited during write transactions (RunModal, Codeunit.Run) - #161
Merged
Jesper Schulz-Wedde (JesperSchulz) merged 10 commits intoSep 29, 2026
Conversation
Contributor
Author
|
@microsoft-github-policy-service agree company="CURABIS ApS" |
Jesper Schulz-Wedde (JesperSchulz)
requested changes
Sep 15, 2026
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Contributor
There was a problem hiding this comment.
Requesting changes for one AL API/routing correctness issue.
The current AL API is Xmlport.Run, not XmlPort.RunModal. Microsoft Learn exposes Xmlport.Run(Integer [, Boolean] [, Boolean] [, var Record]); there is no XMLport RunModal method. Likewise, UseRequestPage(false) is a report-instance method; XMLports use the UseRequestPage = false; object property or the static Xmlport.Run RequestWindow argument. Please update the article's normative bullet, Best Practice/Anti Pattern wording, keywords, and al-performance-review tokens/cue to route actual Xmlport.Run calls. Keep XmlPort.RunModal only in the quoted/explained legacy runtime message. As written, the skill misses real AL XMLport calls and teaches an API that does not compile.
The Page fixture pair and remaining transaction claims check out against current Learn/BCApps. All three exact-head validators pass (frontmatter; deterministic 301-article index; 34 fixtures/17 leaves). Existing checks are green. The head currently conflicts with main in al-performance-review.md after #148; when rebasing, retain both the current job-queue additions and this corrected transaction cue.
… page" from the prompts article A modal page does not behave like Confirm/StrMenu inside a write transaction: the platform refuses Page.RunModal (and Report/XmlPort .RunModal with a request page, and Codeunit.Run with its return value used) with a runtime error instead of holding the lock. The new article documents that guard - verified against Microsoft Learn (Codeunit.Run transaction semantics), microsoft/AL#5452, Microsoft's own Base Application (Commit(); Page.RunModal pattern), and a live reproduction on Business Central 26 quoted verbatim. avoid-user-prompts-inside- transactions.md keeps its scope to the prompts the platform does allow; "modal page" is removed from its list because that case is refused, not stalled. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Message runs asynchronously - it is queued and shown when the calling method ends or another method requests input - so it never pauses the transaction and holds no lock. Only Confirm and StrMenu wait for the user. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…method Mirrors the structure of the platform's own error message so the Report.RunModal and XmlPort.RunModal request-page exceptions are visible at a glance instead of buried in prose. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
"AL methods limited during write transactions: commit before RunModal and Codeunit.Run" - so a developer or agent searching for the runtime error text lands on the one article that covers all four restricted methods. Slug and sample stems renamed to match; keywords gain the error's own phrase and the legacy Form.RunModal name it still uses. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds Page.RunModal / Report.RunModal / XmlPort.RunModal / UseRequestPage to the extracted-token list and one deterministic worklist cue with exclusions, so the article is selected from the RunModal call itself rather than only via a co-located Commit/Modify token. Codeunit.Run in that position is routed to its existing owner article. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…inks XmlPort has no RunModal method (static or instance) - Microsoft Learn confirms only Xmlport.Run(Integer [, Boolean RequestWindow] [, Boolean] [, var Record]). Replaces the invented API with the real one throughout the article, worklist cue, and token list, and explains the platform error message's own "XmlPort.RunModal" wording as the same kind of legacy phrasing already noted for Form.RunModal/RequestForm. Also corrects UseRequestPage(false): that's a Report instance method only, not applicable to XMLport, which uses the UseRequestPage = false object property or Run's RequestWindow argument instead. Fixes the two sample references to use the READ-convention markdown link form (Test-KnowledgeIndex.ps1's Knowledge-Retrieval.ps1 check was failing on plain backtick text). Rebased onto upstream/main to resolve conflicts with microsoft#148's job-queue additions to al-performance-review.md - both sets of worklist tokens and cues are retained.
Michael Dieringer (MichaelDieringer)
force-pushed
the
community-contribution/runmodal-write-transaction-guard
branch
from
September 21, 2026 20:15
9694d05 to
fb896e9
Compare
Contributor
Author
|
Thanks for catching this — you're right,
While rebasing I also noticed the two Rebased onto current |
Michael Dieringer (MichaelDieringer)
added a commit
to Curabis/BCQuality
that referenced
this pull request
Sep 21, 2026
The same plain-backtick "See sample: \`x.good.al\`." form fixed on al-methods-limited-during-write-transactions (PR microsoft#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.
2 tasks
Jesper Schulz-Wedde (JesperSchulz)
requested changes
Sep 22, 2026
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Contributor
There was a problem hiding this comment.
One merge-critical routing gap remains. The article correctly covers request-page-enabled Report.Run and Report.RunModal after writes, but al-performance-review.md tokens/cues recognize only Report.RunModal. A failing Modify(); Report.Run(..., true, ...) path is therefore not reliably worklisted.
Add Report.Run to deterministic routing and exclude calls where the request page is suppressed (Report.Run(..., false, ...) or UseRequestPage(false)). The prior XMLport API/property corrections otherwise appear resolved.
Contributor
|
Nearly there: the prior XMLport/API corrections are sound. The latest review has only one remaining deterministic-routing gap for request-page-enabled Report.Run. |
…rd routing al-performance-review.md's tokens/cues only recognized Report.RunModal, so a failing Modify(); Report.Run(..., true, ...) path was never worklisted even though Report.Run shares the exact same RequestWindow- blocking-dialog mechanism as Report.RunModal (they differ only in whether the report instance is cleared afterward). Added Report.Run to the token list and the targeted cue, with the same request-page- suppressed exclusion, and extended the knowledge article's Description bullet to name both methods explicitly instead of only RunModal.
Contributor
Author
|
Thanks Jesper. Added `Report.Run` alongside `Report.RunModal` to `al-performance-review.md`'s token list and targeted cue (same request-page-suppressed exclusion), and named both methods explicitly in the knowledge article's Description bullet — they share the identical RequestWindow-blocking-dialog mechanism, differing only in whether the report instance is cleared afterward, so the guard and the routing now treat them the same. Frontmatter and knowledge-index validators pass clean. |
Jesper Schulz-Wedde (JesperSchulz)
approved these changes
Sep 22, 2026
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Contributor
There was a problem hiding this comment.
The remaining routing gap is resolved. Request-page-enabled Report.Run is now routed deterministically, with suppressed request-page forms excluded. No new merge-critical issue found.
Jesper Schulz-Wedde (JesperSchulz)
pushed a commit
that referenced
this pull request
Sep 29, 2026
…aking-changes, performance, testing (#156) * 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>
Resolve the performance-review routing conflict by preserving both the write-transaction guard guidance and the latest document-report layout route. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
The merge conflict has been resolved and fully validated. Because GitHub reports Please merge that small sync PR. It applies validated merge commit |
Resolve BCQuality microsoft#161 merge conflicts
Jesper Schulz-Wedde (JesperSchulz)
merged commit Sep 29, 2026
56ce52a
into
microsoft:main
6 of 7 checks passed
Michael Dieringer (MichaelDieringer)
added a commit
to Curabis/BCQuality
that referenced
this pull request
Sep 29, 2026
…crosoft#198, microsoft#202) into document-distribution-batch Resolve evaluation/review-fixtures.json semantically: data-modeling articles list is the union of main's list and this PR's nine articles; everything else is taken from main unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One new
performancearticle,al-methods-limited-during-write-transactions.md, documenting the platform restriction behind the runtime error "The following AL methods are limited during write transactions because one or more tables will be locked: Form.RunModal, Codeunit.Run, Report.RunModal, XmlPort.RunModal." — one explicit line per method with its exact condition (Page.RunModal never; Report/XmlPort.RunModal only with the request page suppressed; Codeunit.Run only when the return value is unused), why the guard exists, and how to structure code so it never triggers.Why the Microsoft layer: this is platform-enforced behavior, not a team convention. Verified against: Microsoft Learn (
Codeunit.Runtransaction semantics — "you must commit first"); a live reproduction on Business Central 26 (2026-09-07,Item.Insert()thenPage.RunModal(Page::"Customer Card")— unrelated table — fails on the RunModal line; message quoted verbatim); Microsoft's own Base Application, which follows theCommit(); Page.RunModal(...)pattern inActivityLog.Table.al,DocumentSendingProfile.Table.al,PaymentServiceSetup.Table.aland others; and microsoft/AL#5452 for the pre-2019 wording. Only the Codeunit.Run leg is documented on Learn; the RunModal legs exist only as the runtime error text — which is exactly why an agent gets this wrong without the file.Relationship to existing articles:
codeunit-run-requires-prior-commit-inside-transaction.mdkeeps ownership of the Codeunit.Run leg (cross-referenced, not duplicated).avoid-user-prompts-inside-transactions.mdkeeps the prompts the platform allows inside a write transaction (Confirm/StrMenu — which therefore silently hold locks); the two words "modal page" are removed from its list, because that case is refused with a runtime error, not stalled.Wiring:
al-performance-review.mdgainsPage.RunModal/Report.RunModal/XmlPort.RunModal/UseRequestPagetokens and one deterministic worklist cue with exclusions (RunModal before every write; request page suppressed), routing Codeunit.Run in that position to its existing owner — per #155's coverage contract.Samples reference real objects only: page 428 "Shipping Agents", table 291 "Shipping Agent" (
Code[10]), Sales Header fields 21 "Shipment Date" and 105 "Shipping Agent Code".Test plan
validate_frontmatter.py --root .— 0 errors, 0 warningsBuild-KnowledgeIndex.ps1— 301 articles, deterministicTest-ReviewFixtures.ps1— 34 cases / 17 leaf domainscompany="CURABIS ApS")microsoft/knowledge/performance/)