Update MessagePack NuGet files and improve type handling - #597
Conversation
…ices into nuget_update
…cept required properties in ctor
Use DerivedTypeUnions to prevent Nerdbank.MessagePack from adding extra discriminators.
|
Warning Review limit reached
Next review available in: 52 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughMessagePack dependencies are updated, surrogate serialization disables derived-type discriminators, PolyType constructors now cover all required properties, and generated test baselines are refreshed. ChangesMessagePack and PolyType integration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant DomainService
participant MessagePackSerializationProvider
participant MessagePackSerializer
DomainService->>MessagePackSerializationProvider: Create serializer
MessagePackSerializationProvider->>MessagePackSerializationProvider: Disable surrogate discriminators
MessagePackSerializationProvider->>MessagePackSerializer: Configure converters and derived-type unions
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenRiaServices MessagePack/PolyType integration to newer package versions and adjusts client-proxy code generation for PolyType-compatible deserialization constructors, with corresponding baseline and packaging updates.
Changes:
- Upgrade
Nerdbank.MessagePackto1.2.36and addPolyType 1.4.1where needed. - Update PolyType deserialization-constructor generation to use required DataMember properties (instead of a workaround using a non-required property).
- Adjust MessagePack serializer configuration to disable derived-type discriminators when surrogates are used; refresh codegen baselines and clean up the
.nuspec.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OpenRiaServices.Hosting.AspNetCore/Framework/OpenRiaServices.Hosting.AspNetCore.csproj | Bumps Nerdbank.MessagePack and adds PolyType dependency. |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePackSerializationProvider.cs | Disables derived-type unions for types using surrogates to avoid discriminator issues. |
| src/OpenRiaServices.Client/Framework/OpenRiaServices.Client.csproj | Bumps Nerdbank.MessagePack dependency. |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/OpenRiaServices.Client.DomainClients.Http.csproj | Bumps Nerdbank.MessagePack and adds PolyType dependency. |
| src/OpenRiaServices.Tools/Framework/DataContractProxyGenerator.cs | Generates PolyType deserialization ctor parameters for all required DataMember properties. |
| src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude | Updates T4 template to emit required-property PolyType constructors. |
| src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpEntityGenerator.cs | Updates generated entity code emitter for required-property PolyType constructors. |
| src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpComplexObjectGenerator.cs | Updates generated complex-object code emitter for required-property PolyType constructors. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/TestProvider_Scenarios_CodeGen.g.cs | Updates expected C# baseline for new ctor shape/required-property selection. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/TestProvider_Scenarios_CodeGen.g.vb | Updates expected VB baseline for new ctor shape/required-property selection. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/TestProvider_Scenarios_CodeGen.g.cs | Updates expected C# baseline (full type names) for new ctor shape. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/TestProvider_Scenarios_CodeGen.g.vb | Updates expected VB baseline (full type names) for new ctor shape. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Mocks/MockCustomers.g.cs | Updates expected C# baseline for required-property ctor changes in mocks. |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Mocks/MockCustomers.g.vb | Updates expected VB baseline for required-property ctor changes in mocks. |
| NuGet/OpenRiaServices.Client.Core.nuspec | Updates package dependency versions and removes commented-out file entries. |
![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.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude`:
- Around line 69-100: Update the generated assignment inside the
requiredProperties loop to reference the property through
CodeGenUtilities.GetSafeName(pd.Name), matching the escaped name used for
parameter declarations and generated property declarations. Preserve the
existing assignment flow and only replace the raw pd.Name target.
🪄 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 Plus
Run ID: c5aa6e5c-7373-412c-ada1-acd600149ed6
📒 Files selected for processing (15)
NuGet/OpenRiaServices.Client.Core.nuspecsrc/OpenRiaServices.Client.DomainClients.Http/Framework/OpenRiaServices.Client.DomainClients.Http.csprojsrc/OpenRiaServices.Client/Framework/OpenRiaServices.Client.csprojsrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePackSerializationProvider.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/OpenRiaServices.Hosting.AspNetCore.csprojsrc/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpComplexObjectGenerator.cssrc/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpEntityGenerator.cssrc/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttincludesrc/OpenRiaServices.Tools/Framework/DataContractProxyGenerator.cssrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Mocks/MockCustomers.g.cssrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Mocks/MockCustomers.g.vbsrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/TestProvider_Scenarios_CodeGen.g.cssrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/TestProvider_Scenarios_CodeGen.g.vbsrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/TestProvider_Scenarios_CodeGen.g.cssrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/TestProvider_Scenarios_CodeGen.g.vb
af63694 to
0f1e6da
Compare
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.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude (1)
80-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the translated property type for constructor parameters.
Lines 180 and 190 back fields and property declarations with
CodeGenUtilities.TranslateType(...), so a requiredSystem.Data.Linq.Binaryproperty is generated asbyte[]in the class while the constructor parameter is emitted asSystem.Data.Linq.Binary, producing a non-compiling assignment.Proposed fix
- string parameterDeclarations = string.Join(", ", requiredProperties.Select(pd => $"{CodeGenUtilities.GetTypeName(pd.PropertyType)} {CodeGenUtilities.GetSafeName(pd.Name)}")); + string parameterDeclarations = string.Join(", ", requiredProperties.Select(pd => $"{CodeGenUtilities.GetTypeName(CodeGenUtilities.TranslateType(pd.PropertyType))} {CodeGenUtilities.GetSafeName(pd.Name)}"));🤖 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.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude` at line 80, Update the constructor parameter generation in the requiredProperties flow to use CodeGenUtilities.TranslateType(pd.PropertyType) before CodeGenUtilities.GetTypeName, matching the translated types used by the generated fields and properties. Keep the existing safe-name handling and parameter formatting unchanged.
🤖 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.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude`:
- Line 80: Update the constructor parameter generation in the requiredProperties
flow to use CodeGenUtilities.TranslateType(pd.PropertyType) before
CodeGenUtilities.GetTypeName, matching the translated types used by the
generated fields and properties. Keep the existing safe-name handling and
parameter formatting unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a66aa37-51aa-4d75-857e-65d7f99601d9
📒 Files selected for processing (1)
src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Dependency updates:
Nerdbank.MessagePackto version 1.2.36 and addedPolyTypeversion 1.4.1 and update code to work with the latest versions.Serialization and constructor generation improvements:
MessagePack/PolyType integration fixes:
DerivedTypeUnionswhen surrogates are used, preventing serialization issues with derived types.(Seems to be a change in behavior of messagepack?)
NuGet packaging cleanup:
netstandard2.0andOpenRiaServices.Client.Web, focusing the package on supported targets and binaries.Summary by CodeRabbit
Bug Fixes
Chores