* knowledge(performance): add community rules for JIT, locks, and false positives
These articles capture BC-specific mechanics agents still invert: partial-record JIT on writes, Reset clearing SetLoadFields, HttpClient inside write transactions, and batched number series, plus negative guidance that stops over-eager Query and IsEmpty "fixes".
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(community/performance): address PR #134 review comments
Fixes technical inaccuracies and behavioral issues raised during review
of the community performance knowledge articles.
avoid-currpage-update-in-onaftergetrecord.md
- Removed OnAfterGetCurrRecord from the Best Practice section. That
trigger is itself implicitly re-entered on every refresh, so placing
CurrPage.Update(false) inside it can re-trigger the very loop the rule
warns about. Only OnAction remains as the recommended location.
batch-number-series-instead-of-getnextno-per-row.md
- Scoped the lock/contention claim to gapless (Normal) series only.
Added explicit statement that Allow Gaps series use NumberSequence
sequences and do not hold the series-line lock, so the anti-pattern
detection signal now excludes Allow Gaps series.
countapprox-for-progress-not-count.bad.al / .good.al
- Changed FindSet(true) to FindSet() in both samples. The loop is
read-only; UpdLock is not needed and was misleading.
countapprox-for-progress-not-count.md
- Qualified the Count() cost claim: it is expensive only when no SIFT
key covers all filtered fields (forcing a SELECT COUNT(*)); a filtered
count with matching SIFT coverage is cheap. Added a parenthetical
noting that SIFT coverage cannot be assumed for arbitrary filters.
dataaccessintent-readonly-on-analytical-objects.md
- Changed bc-version from [all] to ["16.."]. DataAccessIntent was
introduced at runtime 5.0 / BC 16 and has no effect in earlier
versions.
- Added precision to the supported objects: pages must be PageType=API
with Editable=false; for queries, replica routing only applies when
the query is exposed via OData/API, not for AL-to-AL calls.
httpclient-inside-write-transaction-holds-locks.good.al
- Replaced the Commit()-before-HttpClient pattern with a two-codeunit
task-deferral pattern. The write completes inside the caller's
transaction (locks released naturally when it ends); a TaskScheduler
task runs the HTTP call in a separate session where no write-
transaction lock is held.
httpclient-inside-write-transaction-holds-locks.md
- Changed Best Practice to recommend TaskScheduler/job queue deferral
as the primary remedy.
- Added an explicit warning against Commit() as a generic remedy: it
irrevocably commits all prior writes in the current transaction, so a
subsequent failure cannot roll them back. Commit() is appropriate only
at top-level entry points where partial persistence is intentional.
oncompanyopen-subscribers-must-not-do-io.good.al
- Added a ClientType guard so the subscriber exits immediately in
background task sessions (OnAfterLogin fires there too, which would
create an unbounded task chain without the guard).
- Added a TaskScheduler.TaskExists idempotency check to avoid queuing
duplicate tasks on repeated logins.
- Fixed the error-fallback codeunit in CreateTask from a self-reference
to 0 (no error codeunit).
- Added a 60-second delay (CurrentDateTime() + 60000) so the task does
not compete with the login session itself.
prefer-related-table-over-extension-on-hot-ledgers.md
- Changed bc-version from [all] to ["23.."]. The companion-table join
optimisation (single join per base table, automatic exclusion on List/
OData pages with partial records) was introduced in v23.
- Scoped the "join is always paid" claim: since v23 the join is
excluded on List/ListPart/OData pages when no extension field is
loaded under partial-record semantics, but it is still paid on every
posting path and any AL code that accesses an extension field.
skip-setloadfields-on-write-and-transferfields.bad.al / .good.al / .md
- Changed the example scenario from Modify(false) (which is actually
valid with a partial record) to TransferFields+Insert into a temporary
record, which is a documented full-load operation.
- Removed Modify from the list of operations that force a full load.
- Added an explicit note in the .md that Modify itself is not in the
full-load list; SetLoadFields is safe to use before Modify(false).
use-dedicated-lookup-pages-not-full-lists.bad.al
- Added a separate CardPart page definition (50101) to replace the
self-referencing FactBox part that referenced the same list page it
was embedded in. A part cannot refer to its own container page.
validate-on-partial-record-forces-jit.good.al
- Restored SetLoadFields to the good sample so it exercises a partial
record and the contrast with .bad.al is field selection, not the
absence of the feature. The good sample loads both Name and Search
Name (the field Name.OnValidate writes), while the bad sample loads
only Name, causing a JIT reload of Search Name on every Validate call.
* fix(community/performance): correct httpclient and oncompanyopen good samples
httpclient-inside-write-transaction-holds-locks.good.al
- Pass Customer.RecordId as the last argument to CreateTask so the task
is bound to the single customer that was written, not to all customers.
- Declare TableNo = Customer on the task codeunit so the platform loads
the bound record into Rec automatically when OnRun executes.
- Replace the FindSet loop over all customers with Rec."No.", preserving
the one-customer scope of the original SyncCustomerLastName procedure.
oncompanyopen-subscribers-must-not-do-io.good.al
- Replace Session.GetCurrentClientType() with Session.CurrentClientType(),
the correct platform method name.
- Fix the idempotency check: TaskScheduler.TaskExists() requires a Guid,
not a codeunit integer ID. Store the Guid returned by CreateTask in
IsolatedStorage (DataScope::Company) under a fixed key; on the next
login read it back as Text, Evaluate it to Guid, and pass that Guid to
TaskExists so the type matches the method signature.
* fix(community/performance): address review feedback
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
* Add TransferFields SkipFieldsNotMatchingType guidance
* Update transferfields-skip-type-mismatch-can-drop-data.md
* Update transferfields-skip-type-mismatch-can-drop-data.good.al
* Move good sample reference under Best Practice
Aligns the article with the repo convention used by the sibling data-modeling files: the .good.al reference belongs under Best Practice and the .bad.al reference under Anti Pattern. Previously both pointers sat under Anti Pattern, leaving the good-sample reference orphaned in the wrong section.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c998c71-f30b-40f3-b714-87fafed505d8
---------
Co-authored-by: Jesper Schulz <jeschulz@microsoft.com>
Copilot-Session: 3c998c71-f30b-40f3-b714-87fafed505d8
Three related concerns, each proven by executable tests rather than recall.
They were extracted from a real defect that shipped through a six-reviewer
panel undetected, which is the admission test passing on behavior rather than
on theory.
owning-table-must-delete-dependents-in-ondelete
AL has no cascading delete. What makes it missable is an asymmetry: the
platform DOES keep references correct on rename via TableRelation, so a
developer who learns that and generalizes it to delete ships orphans. Also
notes the permission trap (delete rights needed on the dependent table, not
just the parent).
validate-table-relation-false-suppresses-rename-propagation
The non-obvious half. The property name implies input validation only, but
disabling it also switches off rename propagation. Verified against a parent
renamed once while a child held three fields: a normal relation (follows),
the same relation with validation disabled (does NOT follow), and a field
with no relation at all (does not follow) — the third being the control that
proves the test can detect a non-propagating field.
xrec-is-a-before-image-only-in-some-triggers
Corrects both the naive belief that xRec is always the previous record and
the folk rule that it 'only works from a page'. The behavior is per-trigger:
a genuine before-image in OnRename and OnDelete regardless of driver, a
mirror of Rec in OnInsert/OnModify when driven from code, and a real
before-image in those two only when a page drove the write. That last
asymmetry is why an OnModify comparison against xRec passes manual page
testing and silently no-ops in a job queue.
Targets /community per CONTRIBUTING — general BC knowledge, not fork-specific.
Frontmatter validator clean; Test-ReviewFixtures passes (32 cases, 16 leaves).
Move eight net-new rules into the Microsoft layer, remove six overlapping articles, and update review skill discovery and references.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b130227-d418-4bc0-9e7d-ec6a37adf039
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
* Promote security knowledge from community to Microsoft layer
Pure git-mv relocation of the SECURITY domain from the community layer to the Microsoft layer. No content changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address review feedback on security knowledge promotion
- do-not-grant-rights-beyond-a-users-entitlement.md: drop the See sample
reference to a .good.al file that does not exist
- Remove the 'Contributions welcome' boilerplate line from
compose-permission-sets, prefer-oauth2, and protect-sensitive-data
- protect-sensitive-data-in-temporary-tables: remove the pointless
DeleteAll on the locally scoped temp buffer in the good sample and
reword Best Practice to note local buffers are cleaned up automatically
- Drop guard-bulk-operations-with-istemporary from the promotion; it
stays in the community layer pending a decision on whether it is security
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Remove Contributions welcome boilerplate from do-not-grant article for consistency
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Repair broken knowledge references and align review examples with canonical articles. Correct explicit version gates, restore a missing title, and recognize the plugin directory in the root guard.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add 15 community knowledge articles from BC Code Intel ingest
Ingests net-new /community knowledge from BC Code Intelligence, surviving
the admission test, gray-zone salvage, and dedup against the full corpus.
Domains: ui (6), error-handling (3), performance (2), upgrade (1),
appsource (1), security (1), telemetry (1). The two BC24 No. Series
migration drafts are merged into one article.
Adds good/bad AL samples for the clean-fit articles (error-handling,
performance, security, telemetry). UI and appsource remain knowledge-only.
Validator and knowledge-index checks pass (207 articles).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Correct SetLoadFields JIT-load article to match MS docs
The draft claimed accessing an unlisted field "reloads the entire row"
per record. Microsoft's partial-records docs say otherwise: the platform
does an implicit Get that loads the missing field(s), and in a direct var
loop the first JIT updates the enumerator so later iterations do not
re-load. The genuine per-row penalty is the pass-by-value case, where the
copy's enumerator is not updated.
Rewrite the article around JIT loading and the by-value footgun, rename
the slug from ...full-reload to ...jit-load, and fix the good/bad samples
to demonstrate the by-value repetition accurately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Jeremy Vyska <jeremy@sparebrained.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pure git-mv relocation of all 7 performance articles from community/knowledge/performance/ to microsoft/knowledge/performance/. No content changes.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The article claimed SetLoadFields must be called before filters, but
call order has no impact on the resulting query plan. Removing the
article along with its good/bad AL samples rather than rewriting,
since the premise itself is incorrect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 35 articles still in their seed form previously carried a banner
reading "Seed article. ... Domain stewards should expand, restructure,
and refine as needed." For a community preview, that phrasing reads as
"TODO left in production" to first-time visitors.
Replace all three banner variants (performance-seeded, security-seeded,
community-ported) with a single positive invitation:
> Contributions welcome — open a PR to refine or extend this article.
Content and structure of the articles are unchanged; only the leading
quote block differs. Articles that had their banner fully stripped in
the earlier triage pass (the showcase-grade ten) are unaffected.
Most of the corpus — FindSet/SetLoadFields/CalcFields patterns, permission
sets, SingleInstance codeunits, DataClassification, IsolatedStorage,
transaction scope, SecretText — describes BC platform behaviour that is
identical across supported versions. The seed [26..28] range on every
file implied a version-specificity the content does not actually have,
and there was no way to express "applies to every version" in the
schema the way [w1] and [all] already do for countries and
application-area.
Extend the v1 schema with a universal sentinel for bc-version, parallel
to the sentinels already defined for the other dimensions:
bc-version: [all] # applies to every BC version
[all] is mutually exclusive with explicit versions. Range shorthand
([26..28]) and explicit lists ([26, 27, 28]) continue to work for files
genuinely tied to a version-gated API or deprecation.
Update read.md (field definition, matching semantics, partial-context
rule), write.md (default to [all], use ranges only with a concrete
reason), README.md (frontmatter example), and the CI validator. All
forty existing knowledge files and the three action skills convert to
[all]; none of the current content is version-gated. Validator passes.
Remove seven knowledge files whose content is generic software-engineering
guidance that a capable LLM already applies without BCQuality present
(HTTPS-only, secret-leakage-in-errors, no-credentials-in-URLs, silent
security-error swallowing, short transaction scope, HTTP timeouts,
StrSubstNo-vs-concatenation). These fail the remedial-knowledge premise
and dilute the signal of the preview corpus.
Strip the "Seed article — domain stewards should expand" banner from ten
files that are ready to showcase (AA0232/AA0233 rules, FindSet read-only
semantics, SetLoadFields ordering and usage, CalcFields-in-loops,
SecretText end-to-end, DataClassification). The banner remains on files
that still need domain-steward refinement.
Add a "What belongs here" section to the README stating the admission
test: a file exists only if a modern LLM would get something wrong or
miss something without it. Gives contributors a concrete yes/no filter
before they open a PR.
Ports 14 concern-sized articles (8 performance, 6 security) and 25
AL samples from BC Code Intelligence, restructured to BCQuality's v1
schema and layered under /community/knowledge/. Each article is
atomic, under 100 lines, and ships <slug>.good.al and (where the
pattern has a clear anti-example) <slug>.bad.al siblings.
Jesper's microsoft-layer leaves (al-performance-review and
al-security-review) source across every enabled layer via
*/knowledge/<domain>/**, so these additions are picked up by the
existing action skills without any new skill definitions.
Performance (8):
- use-deleteall-for-filtered-bulk-deletion
- call-setloadfields-before-filters
- load-common-fields-before-branching-on-case
- load-only-primary-key-fields-for-reference-work
- omit-filter-only-fields-from-setloadfields
- choose-maintainsiftindex-by-read-write-ratio
- avoid-growing-globals-in-singleinstance-subscribers
- order-case-branches-by-frequency
Security (6):
- classify-every-field-with-dataclassification
- protect-sensitive-data-in-temporary-tables
- guard-bulk-operations-with-istemporary
- compose-permission-sets-with-included-sets
- do-not-grant-rights-beyond-a-users-entitlement
- prefer-oauth2-over-api-keys-for-external-http-calls
Graveyard-bound items (not ported; to be captured in a later
/docs/triage-graveyard.md):
- testfield-performance (soft guidance, low actionability)
- table-event-batch-operation-impact (keep-event-subscribers-lightweight
already carries the core insight)
- Most of /roger-reviewer (AL formatting - frontier-model territory)
- sift-technology-fundamentals (descriptive, not a citable concern)
- bc-telemetry-buddy-* (tooling promotion, not guidance)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
