mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-06 15:16:56 +01:00
knowledge(events): ChangeCompany leaves triggers and trigger-event subscribers running in the calling company (#152)
* knowledge(events): ChangeCompany leaves triggers and trigger-event subscribers running in the calling company ChangeCompany redirects only the data access of a record variable; Learn states that triggers still run in the current company. The database trigger events are raised on every database operation and only pass RunTrigger to the subscriber, so Insert(false) after ChangeCompany still runs every subscriber in the calling company. Generated code either assumes the record 'becomes' a target-company record, or switches RunTrigger off and hand-copies the trigger logic, leaving the subscribers writing to the wrong company; none of the tested runs reached StartSession with the company parameter. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * knowledge(events): qualify StartSession async semantics and fix concurrency-unsafe sample key Addresses PR #152 review: StartSession is a fire-and-forget background session (Ok reports only whether it started, not whether the codeunit succeeded, and errors inside it do not propagate), so the Best Practice now scopes the recommendation and calls out the durable status/error channel a synchronous-success write needs. The good sample's FindLast()+1 entry-number pattern raced under concurrent background sessions; switched to AutoIncrement, which the platform guarantees is unique across concurrent transactions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * knowledge(events): serialize the setup-counter increment; promote article and wire the review skill PR #152 round 3 (JesperSchulz): - The good sample's OnAfterInsertEvent subscriber still raced on the shared "Transfer Setup Good" singleton (Get/increment/Modify); AutoIncrement only protected the request key. Added TransferSetup.LockTable() before Get() to serialize concurrent background sessions. - Promoted changecompany-runs-triggers-in-the-calling-company from community/knowledge/events/ to microsoft/knowledge/events/, and wired ChangeCompany/StartSession/RunTrigger tokens plus a targeted detection cue into microsoft/skills/review/al-events-review.md so a diff containing the anti-pattern reliably worklists this article, preserving the documented RunTrigger=false hand-off exception. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: waldo1001 <12088142+waldo1001@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
parent
d24dc7b14b
commit
b7617fb48a
4 changed files with 219 additions and 1 deletions
|
|
@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially codeunits that publish events or host event subscribers, posting/release/validation routines that should expose extension points, and test codeunits that bind subscribers.
|
||||
- The changed procedures and triggers, weighted toward event publisher methods, methods carrying the `[EventSubscriber(...)]` attribute, routines that raise `OnBefore`/`OnAfter` events, and any procedure that calls `BindSubscription`/`UnbindSubscription`.
|
||||
- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `InternalEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `GlobalVarAccess`, `Isolated`, `local`, `internal`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`).
|
||||
- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `InternalEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `GlobalVarAccess`, `Isolated`, `local`, `internal`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`, `ChangeCompany`, `StartSession`, `RunTrigger`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
@ -65,6 +65,7 @@ The following targeted checks map diff signals to specific `events` articles. Tr
|
|||
- A `RecordRef` event parameter, or a passed-through `xRec`, where a concrete typed record fits — `avoid-loosely-typed-event-parameters`.
|
||||
- A `var IsHandled` added to a pre-existing event rather than introduced through a new `OnBefore` publisher — `do-not-add-ishandled-to-an-existing-event`.
|
||||
- An `if IsHandled then exit;` whose skipped body performs posting, ledger-entry creation, number-series consumption, or integrity/permission validation — `do-not-bypass-critical-operations-with-ishandled`.
|
||||
- A record variable that had `ChangeCompany(<name>)` called on it and is later used with `Insert`, `Modify`, `Delete`, or `Validate`, where the table is not owned by the extension, has triggers that read company data, or has trigger-event subscribers that do not exit on `RunTrigger = false` — `changecompany-runs-triggers-in-the-calling-company`. Do not match a read-only use after `ChangeCompany`, a write with `RunTrigger = false` into an extension-owned table whose triggers do not read company data and whose trigger-event subscribers exit on `RunTrigger = false`, or the parameterless `ChangeCompany()` reset.
|
||||
|
||||
## Action
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue