MessagePack: Fix client serialization for multiple levels of inheritance - #595
Conversation
… class hierarchies with multiple levels will not work
|
FYI @fremo917 |
📝 WalkthroughWalkthroughMessagePack serialization now registers converters for the base ChangesInheritance-aware serialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MessagePackHttpDomainClientFactory
participant ObjectConverterFactory
participant MessagePackSerializer
participant InheritanceTests
MessagePackHttpDomainClientFactory->>ObjectConverterFactory: create and register converters
ObjectConverterFactory-->>MessagePackHttpDomainClientFactory: provide disabled derived-type unions
MessagePackHttpDomainClientFactory->>MessagePackSerializer: configure converters and unions
InheritanceTests->>MessagePackSerializer: load inheritance queries
MessagePackSerializer-->>InheritanceTests: return base and derived entities
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/MessagePackWorkarounds.md`:
- Around line 53-56: Replace the placeholder bullet under “PolyType errors give
poor exception messages” with a concrete, usable reference to the relevant
TypeScript PolyType issue or documentation, including its link; if no valid
reference exists, remove the bullet.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cs`:
- Around line 48-52: Update the serializer construction in the MessagePack HTTP
domain client factory to deduplicate DerivedTypeUnions by BaseType before
combining the converter factory unions with the caller-provided serializer
unions, ensuring the converter factory entry overrides matching existing entries
without duplicate keys. Add a regression test using a serializer that already
defines an explicit union and verify construction succeeds with the intended
union applied.
In `@src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs`:
- Around line 400-401: Update the comment above the query test to describe that
the method loads three queries and verifies their entity counts; remove the
inheritance/custom-method description copied from the preceding test.
🪄 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: 771e1a61-800a-4d9a-8dce-b34e8e17d019
📒 Files selected for processing (4)
src/MessagePackWorkarounds.mdsrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverterFactory.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cssrc/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs
There was a problem hiding this comment.
🟡 Not ready to approve
There are several objective naming/documentation issues (including typos and misleading comments) that should be corrected before merging to keep tests and workaround docs clear and maintainable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the MessagePack HTTP client serialization configuration to better handle polymorphic entity graphs across multiple inheritance levels, and adds an end-to-end test to validate querying across a 3-level inheritance chain.
Changes:
- Extend MessagePack serializer configuration to disable built-in derived-type unions for types handled by the custom
ObjectConverterFactory. - Update
ObjectConverterFactoryto expose derived-type union configuration required for correct polymorphic handling. - Add a new WCF end-to-end async query test covering
City -> CityWithEditHistory -> CityWithInfoinheritance.
File summaries
| File | Description |
|---|---|
| src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs | Adds an async query test to validate client-side deserialization across 3 inheritance levels. |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cs | Updates serializer creation to include derived-type union disabling from ObjectConverterFactory. |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverterFactory.cs | Adds API to emit derived-type union settings to avoid extra discriminators for affected types. |
| src/MessagePackWorkarounds.md | Updates internal documentation with additional notes about PolyType exception-message quality. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs (1)
405-413: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the returned entity types, not only counts.
The middle and most-derived queries only assert that two entities were returned, so incorrect entity types could still pass this test. Add concrete-type assertions, such as checking
editResult.Entities.OfType<CityWithInfo>()andmostDerivedResult.Entities.OfType<CityWithInfo>(), to verify inheritance-aware deserialization.🤖 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/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs` around lines 405 - 413, Extend the assertions in the inheritance test after loading editResult and mostDerivedResult to verify their entities are concrete CityWithInfo instances, using OfType<CityWithInfo>() and asserting the expected count of 2. Keep the existing total-count assertions and baseResult checks 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.
Inline comments:
In `@src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs`:
- Around line 399-400: Update the inheritance query-test comment near the
hierarchy declaration to read “Ensure we can retrieve entities at all levels of
the hierarchy.” Keep the hierarchy description unchanged.
---
Outside diff comments:
In `@src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs`:
- Around line 405-413: Extend the assertions in the inheritance test after
loading editResult and mostDerivedResult to verify their entities are concrete
CityWithInfo instances, using OfType<CityWithInfo>() and asserting the expected
count of 2. Keep the existing total-count assertions and baseResult checks
unchanged.
🪄 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: 1d35d315-d78a-46d0-9780-9cbeaf300ad2
📒 Files selected for processing (1)
src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs
There was a problem hiding this comment.
🟢 Ready to approve
Only minor documentation/comment typos were found; the functional changes are targeted and include coverage for the reported inheritance scenario.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (3)
src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs:409
- Comment grammar: "should be return" should be "should return" (and reads better with a comma).
// CityWithEditHistory is abstract so should be return exactly the same number of entities as CityWithInfo
src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverterFactory.cs:63
- Typo in XML doc: "Nerdabank.MessagePack" should be "Nerdbank.MessagePack" (project/package name), and "built in" should be hyphenated when used as an adjective.
/// In order to prevent Nerdabank.MessagePack from creating additional discriminators we must disable the built in inheritance support for the types.
src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InheritanceTests.cs:400
- Comment grammar: "Ensure we can methods retrieve" reads like a typo and is hard to parse.
This issue also appears on line 409 of the same file.
// Ensure we can methods retrieve entities at all levels of the hierarchy
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Fix client handling of classes with multiple levels of inheritance.
Summary by CodeRabbit
Bug Fixes
Tests