Fix: calculated-field evaluation must not persist via a spurious Update#340
Merged
Conversation
When XrmMockup evaluated a classic calculated field during a Retrieve/RetrieveMultiple, the parsed calc workflow reached its terminal SetAttributeValue node and issued a real orgService.Update of the record. That write was wrong regardless: a calculated field should only compute a value and project it onto the returned entity. The spurious Update fired the update pipeline, bumped modifiedon, and ran UpdateRequestHandler's HasCircularReference guard — which rejects any record legitimately carrying a self-referential lookup (e.g. a systemuser whose createdby is itself), surfacing as the "circular reference" FaultException in the bug report. Fix (smallest change satisfying the constraints): add a suppressWrites parameter to WorkflowTree.Execute. It sets a "SuppressWrites" sentinel into Variables *after* Reset() (which reinitializes Variables), and SetAttributeValue.Execute returns instead of calling orgService.Update when the sentinel is set. ExecuteCalculatedFields passes suppressWrites: true; the computed value is already left in the primaryEntity variable and copied back as before. All other callers (real workflows via WorkflowManager, rollups via CalculateRollupFieldRequestHandler) keep the default false and still persist. ExecuteFormulaFields uses the PowerFx evaluator and never reaches SetAttributeValue, so it needs no change. Removing the write exposed a pre-existing latent NRE in Utility.GetFormattedValueLabel: a Money attribute's formatted value dereferenced transactioncurrencyid without a null check. Previously the spurious Update ran HandleCurrencies and backfilled the currency; without the write, a calculated Money column can be projected onto a record that has no currency, and RetrieveMultiple's SetFormattedValues threw. Guard the Money branch to omit the formatted value when there is no currency, mirroring the tolerant Lookup branch beside it. Regression test (TestMoney.TestCalculatedFieldRetrieveDoesNotPersistAsUpdate): creates a ctx_parent, advances the mock clock, then reads the calculated Money column via both Retrieve and RetrieveMultiple. Before the fix modifiedon jumped forward by the clock advance (proving a spurious Update); after, it is unchanged and the calc value is still projected. Full net8.0 suite: 620 passed, 1 skipped, 0 failed (green across repeated runs). Co-Authored-By: Claude <noreply@anthropic.com> via Conducktor <conducktor@contextand.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an incorrect side effect where classic calculated-field evaluation during Retrieve/RetrieveMultiple persisted changes by issuing a real Update, unintentionally triggering the update pipeline (e.g., modifiedon changes and circular-reference guard faults). The PR introduces a write-suppression mechanism for calculated-field workflow execution and hardens Money formatted-value generation when currency is missing.
Changes:
- Add a
suppressWritesoption toWorkflowTree.Execute, used by calculated-field evaluation to prevent persistence viaSetAttributeValue. - Make
SetAttributeValueskiporgService.Updatewhen write suppression is enabled. - Prevent
RetrieveMultipleformatted-value generation from throwing for Money values withouttransactioncurrencyid, and add a regression test + release notes entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/XrmMockup365Test/TestMoney.cs | Adds regression test ensuring calculated-field evaluation does not bump modifiedon via a spurious Update. |
| src/XrmMockup365/Workflow/WorkflowTree.cs | Adds suppressWrites parameter and suppression sentinel variable for workflow execution. |
| src/XrmMockup365/Workflow/WorkflowNode/SetAttributeValue.cs | Skips persistence (orgService.Update) when suppression sentinel is present. |
| src/XrmMockup365/Internal/Utility.cs | Avoids NRE when formatting Money values without transactioncurrencyid by omitting formatted value. |
| src/XrmMockup365/Core.cs | Executes calculated-field workflow trees with suppressWrites: true. |
| RELEASE_NOTES.md | Documents the calculated-field persistence fix and the Money formatted-value fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reset() does not clear the Variables dictionary, so setting SuppressWritesKey only when suppressWrites was true could leave a stale 'true' on a reused WorkflowTree instance, silently suppressing a later real workflow's Update. Assign the flag on every Execute so behavior depends only on the current call. Co-Authored-By: Claude <noreply@anthropic.com> via Conducktor <conducktor@contextand.com>
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
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.
When XrmMockup evaluated a classic calculated field during a Retrieve/RetrieveMultiple, the parsed calc workflow reached its terminal SetAttributeValue node and issued a real orgService.Update of the record. That write was wrong regardless: a calculated field should only compute a value and project it onto the returned entity. The spurious Update fired the update pipeline, bumped modifiedon, and ran UpdateRequestHandler's HasCircularReference guard — which rejects any record legitimately carrying a self-referential lookup (e.g. a systemuser whose createdby is itself), surfacing as the "circular reference" FaultException in the bug report.
Fix (smallest change satisfying the constraints): add a suppressWrites parameter to WorkflowTree.Execute. It sets a "SuppressWrites" sentinel into Variables after Reset() (which reinitializes Variables), and SetAttributeValue.Execute returns instead of calling orgService.Update when the sentinel is set. ExecuteCalculatedFields passes suppressWrites: true; the computed value is already left in the primaryEntity variable and copied back as before. All other callers (real workflows via WorkflowManager, rollups via CalculateRollupFieldRequestHandler) keep the default false and still persist. ExecuteFormulaFields uses the PowerFx evaluator and never reaches SetAttributeValue, so it needs no change.
Removing the write exposed a pre-existing latent NRE in Utility.GetFormattedValueLabel: a Money attribute's formatted value dereferenced transactioncurrencyid without a null check. Previously the spurious Update ran HandleCurrencies and backfilled the currency; without the write, a calculated Money column can be projected onto a record that has no currency, and RetrieveMultiple's SetFormattedValues threw. Guard the Money branch to omit the formatted value when there is no currency, mirroring the tolerant Lookup branch beside it.
Regression test (TestMoney.TestCalculatedFieldRetrieveDoesNotPersistAsUpdate): creates a ctx_parent, advances the mock clock, then reads the calculated Money column via both Retrieve and RetrieveMultiple. Before the fix modifiedon jumped forward by the clock advance (proving a spurious Update); after, it is unchanged and the calc value is still projected. Full net8.0 suite: 620 passed, 1 skipped, 0 failed (green across repeated runs).