EFCore: Set AllowEmptyStrings=true on client for non nullable string properties#575
Conversation
Daniel-Svensson
changed the title
📝 WalkthroughWalkthroughThis PR makes EF Core codegen emit ChangesRequired String Attribute Generation
InvokeResult refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Pull request overview
Updates EF Core metadata inference so client proxies generate [Required(AllowEmptyStrings=true)] for non-nullable string properties (aligning “required” with non-null rather than non-empty), and bumps the EF Core integration package version to 4.1.0.
Changes:
- Infer
RequiredAttributewithAllowEmptyStrings=truefor non-nullablestringproperties in the EF Core type descriptor. - Update generated baseline files (C# / VB) to reflect the new
[Required(AllowEmptyStrings=true)]output. - Bump
OpenRiaServices.Server.EntityFrameworkCorepackage/assembly version to 4.1.0 and add a changelog entry.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OpenRiaServices.Server.EntityFrameworkCore/Framework/EFCoreTypeDescriptor.cs | Changes inferred RequiredAttribute to allow empty strings for string properties. |
| src/OpenRiaServices.Server.EntityFrameworkCore/Framework/OpenRiaServices.Server.EntityFrameworkCore.csproj | Bumps VersionPrefix/AssemblyVersion to 4.1.0. |
| Changelog.md | Adds release note entry for the change/version. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/EFCoreDbContextScenarios.g.cs | Baseline update for generated [Required(AllowEmptyStrings=true)]. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/EFCoreDbContextScenarios.g.vb | Baseline update for generated RequiredAttribute(AllowEmptyStrings:=true). |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/EFCoreContextScenarios.g.cs | Baseline update for generated [Required(AllowEmptyStrings=true)]. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/EFCoreContextScenarios.g.vb | Baseline update for generated Required(AllowEmptyStrings:=true). |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/EF/Northwind_EFCore.g.cs | Baseline update for generated [Required(AllowEmptyStrings=true)]. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/EF/Northwind_EFCore.g.vb | Baseline update for generated Required(AllowEmptyStrings:=true). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot can you fix the failing test (DomainServices.TestClientCodegen_EFCore_ComplexObject) ? You will need to |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OpenRiaServices.Client/Framework/InvokeResult.cs (1)
37-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd InvokeResult nullability change to the 4.1.0 changelog.
The changelog for EF Core 4.1.0 documents the
AllowEmptyStrings=truecodegen change but does not mention theInvokeResult<T>public API nullability tightening introduced in this PR:
Valueproperty:T?→T- Implicit operator: returns
T?→TThis is a source-level breaking change for nullable-reference-type-aware consumers, who will receive "always true" / nullable-mismatch diagnostics on existing null checks. Add an entry to
Changelog.mdunder the 4.1.0 section documenting this change (the runtime behavior is unchanged, but the typed contract is now stricter).🤖 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.Client/Framework/InvokeResult.cs` around lines 37 - 60, Update Changelog.md under the 4.1.0 section to document the source-level nullability tightening for InvokeResult<T>: note that the public Value property changed from T? to T and the implicit operator on InvokeResult<T> now returns T (was T?), explain this is a nullable-reference-type breaking change for source-level consumers (nullable-mismatch diagnostics) while runtime behavior is unchanged; reference the symbols InvokeResult<T>.Value and the implicit operator on InvokeResult<T> so reviewers can locate the API change.
🤖 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.
Outside diff comments:
In `@src/OpenRiaServices.Client/Framework/InvokeResult.cs`:
- Around line 37-60: Update Changelog.md under the 4.1.0 section to document the
source-level nullability tightening for InvokeResult<T>: note that the public
Value property changed from T? to T and the implicit operator on InvokeResult<T>
now returns T (was T?), explain this is a nullable-reference-type breaking
change for source-level consumers (nullable-mismatch diagnostics) while runtime
behavior is unchanged; reference the symbols InvokeResult<T>.Value and the
implicit operator on InvokeResult<T> so reviewers can locate the API change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 943c24eb-33f5-4d0e-842f-d1378e06d172
📒 Files selected for processing (3)
src/OpenRiaServices.Client/Framework/InvokeResult.cssrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/EFCore_ComplexObject.g.cssrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/EFCore_ComplexObject.g.vb
|
[Required(AllowEmptyStrings=true)]instead of[Required]on client for non nullable string propertiesSummary by CodeRabbit
Bug Fixes
Chores
Documentation