Ensure ChangeSet is always initialized before use#574
Conversation
Removed nullable ChangeSet, added EnsureChangeSetInitialized() to throw if uninitialized, and replaced all null-forgiving usages. Added DomainService_ChangeSetNotInitialized resource string used in the thrown exception.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDomainService.ChangeSet becomes non-nullable and now throws a localized InvalidOperationException if accessed before initialization; all submit-pipeline sites use the non-nullable property. A new resource entry, tests, polyfill, changelog note, and several project target bumps were added. ChangesChangeSet Non-Nullable Contract and Pipeline Migration
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OpenRiaServices.Server/Framework/Data/DomainService.cs`:
- Around line 1023-1034: Add unit tests that assert the new fail-fast guard:
call methods/properties that access ChangeSet before initialization and verify
EnsureChangeSetInitialized throws InvalidOperationException with
Resource.DomainService_ChangeSetNotInitialized; then add a positive test that
initializes _changeSet (or runs the initialization path used in production) and
verifies submit flows (e.g., the submit method/path that uses ChangeSet)
complete without throwing and behave as expected. Target tests at the
EnsureChangeSetInitialized method and the private field _changeSet and exercise
the public submit API that relies on ChangeSet to ensure both the failure and
normal-success scenarios are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af176d1b-d9e7-4911-96ed-dfdefdd7081c
📒 Files selected for processing (3)
src/OpenRiaServices.Server/Framework/Data/DomainService.cssrc/OpenRiaServices.Server/Framework/Data/Resource.Designer.cssrc/OpenRiaServices.Server/Framework/Data/Resource.resx
|
@dotnet-policy-service agree company="Treasury Systems Sweden AB" |
Added ChangeSetAccessorDomainService and related tests to verify ChangeSet accessibility and exception handling before and during initialization and submit operations. Implemented helper methods and assertions for test coverage.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Daniel-Svensson
left a comment
Daniel-Svensson
left a comment
There was a problem hiding this comment.
Generally looks good, can you just update the ChangeSet property in order to allow inlining of it to ensure optimal performance.
Also can you please just add a line to the changelog.md to make it clear that this is a behaviour change (so it has a low risk of being a breaking change in case someone somewhere expects to be able to check the property to see if it is a submit or not)
|
@TS-Jack-Webb it seems builds are failing (might be related to azure deveops image changes) There errors mentions |
It's the projects in VisualStudioExtension that need to be updated, I had to unload them when building the solution locally. I will update them. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/VisualStudio/ItemTemplates/CSharp/DomainServiceClass/DomainServiceClass.csproj (1)
78-83: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider updating the BootstrapperPackage framework version.
The project now targets .NET Framework v4.8, but the BootstrapperPackage still references v4.5. While not critical for template projects, aligning the bootstrapper to reference v4.8 (or removing it if ClickOnce deployment is not needed) would maintain consistency.
📦 Proposed update
- <BootstrapperPackage Include=".NETFramework,Version=v4.5"> + <BootstrapperPackage Include=".NETFramework,Version=v4.8"> <Visible>False</Visible> - <ProductName>Microsoft .NET Framework 4.5 %28x86 and x64%29</ProductName> + <ProductName>Microsoft .NET Framework 4.8 %28x86 and x64%29</ProductName> <Install>true</Install> </BootstrapperPackage>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/VisualStudio/ItemTemplates/CSharp/DomainServiceClass/DomainServiceClass.csproj` around lines 78 - 83, The BootstrapperPackage element in the project file still references .NETFramework,Version=v4.5 (the BootstrapperPackage node and its ProductName) while the project targets .NET Framework 4.8; either update the BootstrapperPackage Include to ".NETFramework,Version=v4.8" and adjust the ProductName to "Microsoft .NET Framework 4.8 (x86 and x64)" to match the target framework, or remove the entire BootstrapperPackage element if ClickOnce/bootstrapper deployment is not required.src/VisualStudio/ItemTemplates/CSharp/AuthenticationDomainService/AuthenticationDomainService.csproj (1)
78-82: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider updating the BootstrapperPackage framework version.
The project now targets .NET Framework v4.8, but the BootstrapperPackage still references v4.5. While not critical for template projects, aligning the bootstrapper to reference v4.8 (or removing it if ClickOnce deployment is not needed) would maintain consistency.
📦 Proposed update
- <BootstrapperPackage Include=".NETFramework,Version=v4.5"> + <BootstrapperPackage Include=".NETFramework,Version=v4.8"> <Visible>False</Visible> - <ProductName>Microsoft .NET Framework 4.5 %28x86 and x64%29</ProductName> + <ProductName>Microsoft .NET Framework 4.8 %28x86 and x64%29</ProductName> <Install>true</Install> </BootstrapperPackage>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/VisualStudio/ItemTemplates/CSharp/AuthenticationDomainService/AuthenticationDomainService.csproj` around lines 78 - 82, The BootstrapperPackage entry still references .NETFramework,Version=v4.5 and a ProductName for .NET 4.5; update the BootstrapperPackage Include value to ".NETFramework,Version=v4.8" and adjust the ProductName to "Microsoft .NET Framework 4.8 %28x86 and x64%29" to match the project target, or remove the entire BootstrapperPackage element (the <BootstrapperPackage> block) if ClickOnce/bootstrapper deployment is not required.src/OpenRiaServices.Server/Framework/Data/DomainService.cs (1)
500-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
_changeSetassignment after operation validation.At Line 501,
_changeSetis assigned beforeEnsureInitialized()andCheckOperationType(...). If validation fails, the instance still keeps a non-null_changeSet, which can makeChangeSetappear initialized in an invalid operation context.Suggested fix
public virtual async ValueTask<bool> SubmitAsync(ChangeSet changeSet, CancellationToken cancellationToken) { try { ArgumentNullException.ThrowIfNull(changeSet); - this._changeSet = changeSet; this.EnsureInitialized(); this.CheckOperationType(DomainOperationType.Submit); + this._changeSet = changeSet; this.ResolveOperations();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OpenRiaServices.Server/Framework/Data/DomainService.cs` around lines 500 - 505, Currently _changeSet is set before validation, so if EnsureInitialized() or CheckOperationType(DomainOperationType.Submit) fail the instance will retain a non-null _changeSet; to fix, keep the null-check ArgumentNullException.ThrowIfNull(changeSet) first, then call this.EnsureInitialized() and this.CheckOperationType(DomainOperationType.Submit), then assign this._changeSet = changeSet, and finally call this.ResolveOperations() so ResolveOperations can operate on a valid, already-validated _changeSet.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Changelog.md`:
- Line 19: Replace the redundant sentence about ChangeSet with a concise
phrasing: change the line that mentions "ChangeSet now throws an
`InvalidOperationException` if accessed outside of a submit operation, eg. in
the cases where `ChangeSet` previously would have returned `null`." to a shorter
sentence such as "ChangeSet now throws an `InvalidOperationException` when
accessed outside of a submit operation." referencing the ChangeSet symbol and
the InvalidOperationException.
In `@src/OpenRiaServices.Server/Framework/Data/DomainService.cs`:
- Around line 227-235: The code currently dereferences
this.ServiceContext.OperationType when _changeSet is null which can call
EnsureInitialized() and throw the wrong exception; fix by avoiding
ServiceContext access—use the backing field or a safe conditional access for the
operation type and pass that into ThrowChangeSetNotInitialized (e.g. use
this._serviceContext?.OperationType ?? default(DomainOperationType)) or change
ThrowChangeSetNotInitialized to not rely on ServiceContext, and update the
CallSite in the ChangeSet getter (symbols: ChangeSet getter, _changeSet,
ServiceContext.OperationType, _serviceContext, ThrowChangeSetNotInitialized,
DomainOperationType) so the InvalidOperationException thrown is the intended
DomainService_ChangeSetNotInitialized.
---
Outside diff comments:
In `@src/OpenRiaServices.Server/Framework/Data/DomainService.cs`:
- Around line 500-505: Currently _changeSet is set before validation, so if
EnsureInitialized() or CheckOperationType(DomainOperationType.Submit) fail the
instance will retain a non-null _changeSet; to fix, keep the null-check
ArgumentNullException.ThrowIfNull(changeSet) first, then call
this.EnsureInitialized() and
this.CheckOperationType(DomainOperationType.Submit), then assign this._changeSet
= changeSet, and finally call this.ResolveOperations() so ResolveOperations can
operate on a valid, already-validated _changeSet.
In
`@src/VisualStudio/ItemTemplates/CSharp/AuthenticationDomainService/AuthenticationDomainService.csproj`:
- Around line 78-82: The BootstrapperPackage entry still references
.NETFramework,Version=v4.5 and a ProductName for .NET 4.5; update the
BootstrapperPackage Include value to ".NETFramework,Version=v4.8" and adjust the
ProductName to "Microsoft .NET Framework 4.8 %28x86 and x64%29" to match the
project target, or remove the entire BootstrapperPackage element (the
<BootstrapperPackage> block) if ClickOnce/bootstrapper deployment is not
required.
In
`@src/VisualStudio/ItemTemplates/CSharp/DomainServiceClass/DomainServiceClass.csproj`:
- Around line 78-83: The BootstrapperPackage element in the project file still
references .NETFramework,Version=v4.5 (the BootstrapperPackage node and its
ProductName) while the project targets .NET Framework 4.8; either update the
BootstrapperPackage Include to ".NETFramework,Version=v4.8" and adjust the
ProductName to "Microsoft .NET Framework 4.8 (x86 and x64)" to match the target
framework, or remove the entire BootstrapperPackage element if
ClickOnce/bootstrapper deployment is not required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7387996-b1a7-4cf7-b92d-484e5e608178
📒 Files selected for processing (9)
Changelog.mdsrc/OpenRiaServices.Server/Framework/Data/DomainService.cssrc/OpenRiaServices.Server/Framework/Data/Resource.Designer.cssrc/OpenRiaServices.Server/Framework/Data/Resource.resxsrc/OpenRiaServices.Server/Test/DomainServiceTests.cssrc/VisualStudio/ItemTemplates/CSharp/AuthenticationDomainService/AuthenticationDomainService.csprojsrc/VisualStudio/ItemTemplates/CSharp/DomainServiceClass/DomainServiceClass.csprojsrc/VisualStudio/Templates/CSharp/BusinessApplication/BusinessApplicationProjectTemplate.csprojsrc/VisualStudio/Templates/CSharp/RIAServicesLibrary/OpenRiaServicesLibrary.csproj
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens DomainService.ChangeSet semantics so it is no longer nullable and will throw a localized InvalidOperationException when accessed before it has been initialized as part of submit processing. It also updates related templates/changelog and adds tests intended to validate the new behavior.
Changes:
- Make
DomainService.ChangeSetnon-nullable and throw with a new localized resource string when uninitialized. - Add tests for ChangeSet access before/after submit, plus supporting test-only DomainService.
- Update VS project/item templates to target .NET Framework 4.8 and document the behavior change in the changelog.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/VisualStudio/Templates/CSharp/RIAServicesLibrary/OpenRiaServicesLibrary.csproj | Template target framework bumped to .NET Framework 4.8. |
| src/VisualStudio/Templates/CSharp/BusinessApplication/BusinessApplicationProjectTemplate.csproj | Template target framework bumped to .NET Framework 4.8. |
| src/VisualStudio/ItemTemplates/CSharp/DomainServiceClass/DomainServiceClass.csproj | Item template target framework bumped to .NET Framework 4.8. |
| src/VisualStudio/ItemTemplates/CSharp/AuthenticationDomainService/AuthenticationDomainService.csproj | Item template target framework bumped to .NET Framework 4.8. |
| src/OpenRiaServices.Server/Test/DomainServiceTests.cs | Adds tests and a helper DomainService to validate ChangeSet access rules. |
| src/OpenRiaServices.Server/Framework/Data/Resource.resx | Adds localized message for uninitialized ChangeSet access. |
| src/OpenRiaServices.Server/Framework/Data/Resource.Designer.cs | Regenerated strongly-typed resources for the new string. |
| src/OpenRiaServices.Server/Framework/Data/DomainService.cs | Implements throwing ChangeSet accessor and removes null-forgiving usages. |
| src/OpenRiaServices.Client/Framework/Polyfills.cs | Adds StackTraceHiddenAttribute polyfill for non-NET targets. |
| Changelog.md | Documents the breaking behavioral change (throw instead of null). |
Files not reviewed (1)
- src/OpenRiaServices.Server/Framework/Data/Resource.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OpenRiaServices.Server/Framework/Data/DomainService.cs`:
- Around line 224-240: The ChangeSet getter currently only checks _changeSet !=
null; update it to also verify the current operation is a submit by checking the
private field _serviceContext.Operation (not the ServiceContext property) and
throw the same InvalidOperationException
(Resource.DomainService_ChangeSetNotInitialized) via the existing
ThrowChangeSetNotInitialized helper when either _changeSet is null or the
operation is not a submit; keep the helper signature and attributes unchanged so
the exception contract and stacktrace behavior remain identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3eede25a-22b1-4521-b27f-4117f2774b22
📒 Files selected for processing (6)
Changelog.mdsrc/OpenRiaServices.Client/Framework/Polyfills.cssrc/OpenRiaServices.Server/Framework/Data/DomainService.cssrc/OpenRiaServices.Server/Framework/Data/Resource.Designer.cssrc/OpenRiaServices.Server/Framework/Data/Resource.resxsrc/OpenRiaServices.Server/Test/DomainServiceTests.cs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Removed nullable ChangeSet, it now throws if _changeSet is
null, and replaced all null-forgiving usages. Added DomainService_ChangeSetNotInitialized resource string used in the thrown exception.Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores