Add new transport based on MessagePack#591
Open
Daniel-Svensson wants to merge 58 commits into
Open
Conversation
Co-authored-by: Daniel-Svensson <3167509+Daniel-Svensson@users.noreply.github.com>
Co-authored-by: Daniel-Svensson <3167509+Daniel-Svensson@users.noreply.github.com>
Co-authored-by: Daniel-Svensson <3167509+Daniel-Svensson@users.noreply.github.com>
Co-authored-by: Daniel-Svensson <3167509+Daniel-Svensson@users.noreply.github.com>
* Parameter passing WORKS * DateTime passing don't throw (Need to investigate)
…ks working. 365 tests are now passing (with submit not yet working) , 323 failing
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MessagePack wire-format support across hosting and client domain services, including envelopes, serializers, transport wiring, type-shape filtering, surrogate/codegen updates, and validation coverage. ChangesMessagePack Serialization
Sequence Diagram(s)sequenceDiagram
participant DomainContext
participant MessagePackHttpDomainClientFactory
participant MessagePackHttpDomainClient
participant MessagePackRequestSerializer
participant RequestSerializer
DomainContext->>MessagePackHttpDomainClientFactory: create client
MessagePackHttpDomainClientFactory-->>DomainContext: MessagePackHttpDomainClient
DomainContext->>MessagePackHttpDomainClient: query / invoke / submit
MessagePackHttpDomainClient->>MessagePackRequestSerializer: serialize request envelope
MessagePackRequestSerializer->>RequestSerializer: write query / invoke / error response
RequestSerializer-->>MessagePackRequestSerializer: response envelope
MessagePackRequestSerializer-->>MessagePackHttpDomainClient: HTTP response body
MessagePackHttpDomainClient-->>DomainContext: result or fault
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+423
to
+426
| if (_inner.HasSetter) | ||
| return _inner.GetSetter(); | ||
| else | ||
| return (ref TDeclaringType instance, TPropertyType value) => { }; |
| // If the name is new then should a separate property be added | ||
| // | ||
|
|
||
| string _name = (inner.AttributeProvider.GetCustomAttribute<IncludeAttribute>() is IncludeAttribute includeAttribute |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 31
🤖 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.Client.DomainClients.Http/Framework/MessagePack/Converters/ChangeSetEntryConverter.cs`:
- Around line 86-88: The ChangeSetEntryConverter class has a wire-contract
mismatch where EntityActions is serialized at lines 163-165 but discarded during
deserialization at lines 86-88. Replace the reader.Skip(context) call in the
EntityActions case statement (lines 86-88 in the anchor location) with proper
deserialization logic that reads the EntityActions value from the message pack
stream and assigns it to the corresponding property. The serialization code at
lines 163-165 (sibling location) requires no changes; it correctly writes the
EntityActions field. Ensure the deserialization reads the same structure that is
being written to maintain the wire contract.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/MessagePackMethodParametersConverter.cs`:
- Around line 28-35: The MessagePackMethodParametersConverter is retrieving
method parameter type information from ambient SerializationContext state using
GetMethodParameters(context), but the serialized value object already carries
MethodParameters directly. Replace all calls to GetMethodParameters(context)
with direct access to value.MethodParameters throughout the class to avoid
mismatches and potential runtime errors. This change should be applied in the
deserialization logic where GetMethodParameters(context) is called at lines
28-35, and also at the other affected locations mentioned (lines 53-60 and
65-66) to ensure consistent use of the value's own MethodParameters rather than
relying on ambient context state.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverter.cs`:
- Around line 79-83: The variable `surrogateType` referenced in the
NET10_0_OR_GREATER conditional branch is undefined; replace it with `type`,
which is the correct variable declared earlier in the method. This assignment
should use the existing `type` variable (not a new `surrogateType` variable)
when assigning the result from the discriminator lookup in the .NET 10.0 or
greater code path.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Envelopes.cs`:
- Around line 11-137: The internal API types and their properties in this file
are missing XML documentation comments, which violates the coding guideline that
new public, protected, and internal APIs should have XML documentation. Add XML
documentation comments (using /// syntax) to all the following: the abstract
class MessagePackRequestEnvelopeBase and its Parameters property, the sealed
classes MessagePackQueryRequestEnvelope, MessagePackInvokeRequestEnvelope, and
MessagePackSubmitRequestEnvelope with their respective properties, the generic
sealed class ChangeSetEntry<T> and all of its properties (Entity,
OriginalEntity, StoreEntity, Id, HasMemberChanges, Operation, EntityActions,
ValidationErrors, ConflictMembers, IsDeleteConflict, Associations,
OriginalAssociations), the abstract class MessagePackResponseEnvelopeBase with
its Fault property and GetResult method, and the sealed response envelope
classes MessagePackQueryResponseEnvelope<TResult>,
MessagePackInvokeResponseEnvelope<TResult>, and
MessagePackSubmitResponseEnvelope with their Result properties and GetResult
method implementations.
- Around line 30-39: The parameterless constructor of ChangeSetEntry<T> at line
30 does not initialize the readonly field _changeSetEntry, while the
parameterized constructor at lines 35-39 does. This creates a code path where
_changeSetEntry remains uninitialized and null, causing potential null reference
exceptions when accessing properties. Fix this by initializing _changeSetEntry
on every constructor path—either by adding a field initializer with a default
value to the _changeSetEntry field declaration, or by adding initialization
logic in the parameterless constructor body to ensure the field is assigned
before returning.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/MessagePackHttpDomainClient.cs`:
- Around line 156-162: The while loop in the GetResponseEnvelopeType method that
walks up the entity base type hierarchy lacks a null check, which can cause a
runtime failure if the entity type doesn't have Entity in its base class chain.
Add a null check to the while condition to guard against entityType.BaseType
becoming null before reaching the Entity type, preventing the loop from walking
past the intended hierarchy and causing a NullReferenceException.
- Around line 106-118: The media type validation in MessagePackHttpDomainClient
is coupled with the success status code check, allowing 2xx responses with
incorrect content type to bypass validation and reach MessagePack
deserialization. Separate the media type validation from the status code check
by first validating that the response content type matches the expected
MediaType independently, and then handle status code errors separately. This
ensures that any response with an incorrect content type is rejected before
attempting deserialization, regardless of the HTTP status code.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/MessagePackMethodParameters.cs`:
- Around line 7-17: The MessagePackMethodParameters class and its members are
missing XML documentation comments required by coding guidelines. Add XML
documentation comments to the class MessagePackMethodParameters, its constructor
MessagePackMethodParameters(MethodParameters methodParameters,
IDictionary<string, object> values), and its two properties Values and
MethodParameters. Each XML doc should clearly describe the purpose and usage of
the class, constructor, and properties using the standard summary tags format.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cs`:
- Around line 100-105: The loop in ComputeKnownTypeSet that iterates through
base types assumes every traversed baseType already exists as a key in the
closure dictionary, but this is not guaranteed and will throw
KeyNotFoundException when accessing closure[baseType] on line 104. Before
accessing closure[baseType], check if the key exists in the dictionary using
closure.ContainsKey(baseType) or closure.TryGetValue(), and if the key does not
exist, initialize it (typically as an empty HashSet<Type>) before calling
UnionWith to add the known types.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/DataContractRequestSerializer.cs`:
- Line 363: The WriteResponseAsync method has an unused operation parameter in
its signature that is never referenced in the method body. Remove the
DomainOperationEntry operation parameter from the WriteResponseAsync method
signature, then update all three call sites of WriteResponseAsync to remove the
_operation argument when invoking the method, as mentioned in the suggested
signature simplification.
- Around line 399-407: The override methods WriteInvokeResponseAsync and
WriteQueryResponseAsync in DataContractRequestSerializer are ignoring the
operation parameter passed to them and instead using the constructor-injected
_operation field. Replace the calls to WriteResponseAsync in both methods to use
the operation parameter instead of _operation to maintain consistency with the
base class API contract and other implementations like
MessagePackRequestSerializer.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/ChangeSetEntryConverter.cs`:
- Around line 81-83: The ChangeSetEntryConverter is not handling the case where
EntityActions is encountered before Entity in the MessagePack map
deserialization. The call to ReadEntityActions on line 82 depends on
result.Entity being populated to determine the type, but map key order is not
guaranteed. Restructure the deserialization logic to either defer processing
EntityActions until after Entity has been deserialized, or store the
EntityActions data temporarily and process it in a second pass after all map
entries have been read. This ensures the converter can handle any key ordering
in valid payloads.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/MethodParametersConverter.cs`:
- Around line 35-40: The ChangeSet type remapping logic that converts ChangeSet
to IEnumerable<ChangeSetEntry> exists only at lines 35-40 in the sync Read
method, but this same remapping is missing at lines 105-107 and lines 139-139 in
the async paths. Apply the same ChangeSet type remapping logic from the sync
Read method (checking if type equals typeof(ChangeSet) and reassigning to
typeof(IEnumerable<ChangeSetEntry>)) to the code at lines 105-107 and lines
139-139 to ensure consistent parameter type handling across all
serialization/deserialization paths.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/SurrogateConverter.cs`:
- Around line 43-50: The SurrogateConverter class has an inconsistency where
`_typeLookup` for the `T == object` path uses all SurrogateTypes, but
`_surrogateFactory` is built from `knownTypes` that exclude complex types
because the line adding description.ComplexTypes is commented out. This causes
deserialization to succeed for types that cannot be serialized back. Uncomment
the line that adds description.ComplexTypes to the allKnownTypes set in the code
block where EntityKnownTypes are being unioned, ensuring that both the read and
write type maps include all necessary types.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProvider.cs`:
- Around line 15-27: Multiple newly introduced API members across several files
are missing required XML documentation comments. Add XML documentation comments
to the following files according to coding guidelines: (1) In
src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProvider.cs
lines 15-27, add summary and parameter documentation for the
FilteredTypeShapeProvider class, its constructor, and the GetTypeShape method;
(2) In
src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePackSerializationProvider.cs
lines 15-37, add XML documentation for the new internal class, constructor, and
methods; (3) In
src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackRequestSerializer.cs
lines 14-35, add XML documentation for the new internal serializer API surface
including class, constructor, and methods; (4) In
src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/ChangeSetEntryConverter.cs
lines 48-55, add XML documentation for the new internal converter API surface;
(5) In
src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cs
lines 105-108, add XML documentation for the new internal helper method. Ensure
each documentation block includes appropriate summary tags and parameter
descriptions as applicable.
- Around line 392-397: The Name property at line 396 returns _inner.Name instead
of the computed _name field, so the projection aliasing logic that extracts
MemberName from IncludeAttribute is never actually used. Fix this by changing
the Name property getter to return _name instead of _inner.Name, so that when a
projection with an alias is detected, the aliased name is properly exposed
through the property.
- Around line 186-187: The code dereferences property.MemberInfo.Name without
first checking if property.MemberInfo is null. Add a null guard to check that
property.MemberInfo is not null before attempting to access its Name property.
This will prevent a NullReferenceException when a shape has no backing
MemberInfo.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProviderOptions.cs`:
- Around line 3-14: Add XML documentation comments to all public API elements in
the file. Document the AssociationMemberSerializationMode enum with a summary
explaining its purpose, then document its enum values Exclude and Include.
Document the FilteredTypeShapeProviderOptions class with a summary of its
purpose, the Default static property explaining what it represents, and the
AssociationMemberSerializationMode property explaining what it controls and its
default behavior. Follow standard XML doc format with summary tags for each
element.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackResponseEnvelopeBase.cs`:
- Around line 12-93: Add XML documentation comments to all internal envelope API
surfaces in the file. The classes MessagePackResponseEnvelopeBase,
MessagePackQueryResponseEnvelope<TResult>,
MessagePackInvokeResponseEnvelope<TResult>, MessagePackSubmitResponseEnvelope,
MessagePackRequestEnvelopeBase, MessagePackQueryRequestEnvelope,
MessagePackInvokeRequestEnvelope, MessagePackSubmitRequestEnvelope, and
MethodParameters, along with all their public properties (Fault, Result,
Parameters, QueryOptions, IncludeTotalCount, Values), are missing XML
documentation comments. Add triple-slash documentation comments above each class
definition and each public property to document their purpose and usage.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cs`:
- Around line 22-27: The attribute lookup in the MessagePackUtility class is
searching the wrong object within the base type traversal loop. In the while
loop that iterates through base types, the GetCustomAttributes call should be
made on the current baseType being examined, not repeatedly on the original
type. Additionally, the comparison in the FirstOrDefault predicate should match
against baseType instead of type to properly identify DerivedTypeShapeAttribute
declarations on each base type in the inheritance chain.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MimeTypes.cs`:
- Line 7: The MessagePack constant defined in the MimeTypes class is missing XML
documentation. Add a summary XML documentation comment above the MessagePack
constant that describes what this MIME type represents, ensuring it follows the
project's documentation guidelines for public API members.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/MessagePack/FilteredTypeShapeProviderTests.cs`:
- Around line 116-134: The union type validation in the else if block for
unionType is being skipped with a bare return statement at line 120, leaving the
validation logic unreachable and untracked. Replace the Console.WriteLine and
early return with either a proper test skip mechanism appropriate for the test
framework being used (such as Assert.Inconclusive or Assert.Skip depending on
the testing framework), or implement the complete union type validation by
uncommenting and completing the TODO code at lines 122-133 that validates
derived types match union cases. Choose based on whether union type validation
is currently required for the MessagePack contract or should be deferred.
- Around line 65-72: Remove the Debugger.Break() call from the conditional block
in FilteredTypeShapeProviderTests.cs. The entire if statement that checks for
type == typeof(Cities.City) and contains the Debugger.Break() call should be
deleted, as debug breakpoints should not be committed to the test suite as they
will cause test hangs in non-interactive CI environments.
- Around line 79-144: The AssertTypesAreEqual method accumulates error messages
when property or type mismatches are detected but only logs them via
Console.WriteLine without actually failing the test. Replace the current error
handling at the end of the method (in the final if-else block checking
string.IsNullOrEmpty(errors)) with a test assertion that will fail the test when
errors is not empty, ensuring that property mismatches cause the test to fail
rather than silently pass.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/SerializationOptionsTests.cs`:
- Around line 264-271: The ReadMessagePackInvokeResponse method has a misleading
signature that accepts a resultType parameter but never uses it, always
hardcoding a string return type instead. Since this test helper is only used for
string results, remove the unused resultType parameter from the method signature
and optionally rename the method to clarify its string-only purpose (for
example, ReadMessagePackInvokeResponseString). Update any call sites that pass a
resultType argument to remove that argument.
In
`@src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DataContractSurrogateGenerator.cs`:
- Around line 215-225: The parameterless constructor check on the condition is
performed but then the result is not reused when emitting IL code. Replace the
duplicate call to type.GetConstructor(Type.EmptyTypes) in the Emit call within
the true branch with the already-captured ctor variable from the pattern match.
Additionally, the else branch currently contains only a TODO comment without
proper error handling, which leaves the wrapper field uninitialized and can
cause runtime failures. Replace the TODO with appropriate exception throwing to
indicate that a parameterless constructor is required for surrogate type
generation, ensuring proper error signaling instead of silently leaving the
wrapper uninitialized.
In `@src/Test/AspNetCoreWebsite/MessagePack/CustomComparerProvider.cs`:
- Around line 6-20: The CustomComparerProvider class and its public API methods
(GetComparer and GetEqualityComparer) are missing XML documentation comments
required by repository standards. Add XML doc comments with summary tags to the
CustomComparerProvider class to describe its purpose, then add summary tags to
both the GetComparer<T> and GetEqualityComparer<T> methods to document what they
do. Additionally, include typeparam documentation for the generic type parameter
T in both methods to explain what T represents.
In `@src/Test/AspNetCoreWebsite/MessagePack/XElementConverter.cs`:
- Around line 10-15: The XML documentation for the XElementConverter class
currently incorrectly describes it as converting Binary objects and references
the wrong package. Update the summary section to indicate that this converter
handles XElement objects instead of Binary, and update the remarks section to
reference the correct package (System.Xml.Linq) that contains XElement rather
than the System.Data.Linq.Data package mentioned for Binary.
In `@src/Test/OpenRiaservices.EndToEnd.AspNetCore.Test/Main.cs`:
- Around line 59-67: The MessagePackSerializer in the msgPackSerializer variable
initialization is missing the BinaryConverter in its Converters list. The
Converters array currently only contains XElementConverter, but the server-side
configuration in Program.cs includes both XElementConverter and BinaryConverter.
Add new BinaryConverter() to the Converters list alongside the existing
XElementConverter() to ensure Binary fields (such as Northwind.Picture,
Northwind.Photo, and TestPredefined operations) can be properly deserialized in
NET10 builds.
In `@src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/CustomMethodTests.cs`:
- Line 515: In the TODO comment that starts with "TODO; Make a way of
validaing...", fix the following errors: replace the semicolon after TODO with a
colon, correct the misspelling "validaing" to "validating", apply proper casing
to "messagepack" as "MessagePack", change the verb "handle" to "handles" for
correct subject-verb agreement, and hyphenate the compound adjective "client
side" as "client-side".
- Around line 518-519: The MessagePackSerializationException assertion in the
code block currently only verifies the exception type using
Assert.IsInstanceOfType, but unlike the DataContract path validation (lines
521-522), it does not validate the error message content. Add a second assertion
after the type check for the MessagePackSerializationException to verify that
the exception message contains relevant context about the missing domain
operation metadata, ensuring consistency with the DataContract test path and
improving diagnostics when the test fails.
🪄 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: 7cdc5a7a-ab12-40e6-b55e-7884caf7e536
📒 Files selected for processing (52)
docs/messagepack-serialization.mdsrc/MessagePackWorkarounds.mdsrc/OpenRiaServices.Client.DomainClients.Http/Framework/Http/HttpDomainClient.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/Http/MetodParameters.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ChangeSetEntryConverter.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/MessagePackMethodParametersConverter.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverter.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Envelopes.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/MessagePackHttpDomainClient.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/MessagePackMethodParameters.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/OpenRiaServices.Client.DomainClients.Http.csprojsrc/OpenRiaServices.Client/Framework/ComplexObject.cssrc/OpenRiaServices.Client/Framework/Entity.cssrc/OpenRiaServices.Client/Framework/OpenRiaServices.Client.csprojsrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/OpenRiaServicesConfigurationBuilder.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/OpenRiaServicesOptionsBuilder.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/InvokeOperationInvoker.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/QueryOperationInvoker.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/SubmitOperationInvoker.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/DataContractRequestSerializer.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/ChangeSetEntryConverter.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/MethodParametersConverter.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/SurrogateConverter.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProvider.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProviderOptions.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackRequestSerializer.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackResponseEnvelopeBase.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePackSerializationOptions.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePackSerializationProvider.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MimeTypes.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/SerializationProvider.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/OpenRiaServices.Hosting.AspNetCore.csprojsrc/OpenRiaServices.Hosting.AspNetCore/Framework/README.mdsrc/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/MessagePack/FilteredTypeShapeProviderTests.cssrc/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/SerializationOptionsTests.cssrc/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DataContractSurrogateGenerator.cssrc/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cssrc/OpenRiaServices.Server/Framework/Data/DomainService.cssrc/OpenRiaServices.Server/Framework/Data/DomainServiceDescription.cssrc/OpenRiaServices.Server/Framework/Data/KnownTypeUtilities.cssrc/Test/AspNetCoreWebsite/MessagePack/BinaryConverter.cssrc/Test/AspNetCoreWebsite/MessagePack/CustomComparerProvider.cssrc/Test/AspNetCoreWebsite/MessagePack/XElementConverter.cssrc/Test/AspNetCoreWebsite/Program.cssrc/Test/AspNetCoreWebsite/Properties/launchSettings.jsonsrc/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Mocks/MockCustomers.g.cssrc/Test/OpenRiaservices.EndToEnd.AspNetCore.Test/Main.cssrc/Test/OpenRiaservices.EndToEnd.AspNetCore.Test/OpenRiaservices.EndToEnd.AspNetCore.Test.csprojsrc/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/ComplexTypeTests.cssrc/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/CustomMethodTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@NuGet/OpenRiaServices.Client.Core.nuspec`:
- Around line 20-31: The System.IO.Hashing package version is declared as 10.0.8
in the nuspec file but referenced as version 10.0.9 in the
OpenRiaServices.Client.DomainClients.Http.csproj project file, creating a
version mismatch. Locate the System.IO.Hashing package reference in the
OpenRiaServices.Client.DomainClients.Http.csproj file and update its version
from 10.0.9 to 10.0.8 to align with the nuspec declaration and prevent
transitive dependency conflicts for package consumers.
- Around line 20-31: The System.IO.Hashing package version in the nuspec file is
declared as 10.0.8 but the actual project dependency uses 10.0.9, causing a
mismatch. Update all three System.IO.Hashing dependency declarations across the
targetFramework groups for net472, net8.0, and net8.0-windows7.0 from version
10.0.8 to version 10.0.9 to align the nuspec with the actual project
dependencies. Nerdbank.MessagePack version 1.2.4 should remain unchanged as it
is correct.
In
`@src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpComplexObjectGenerator.cs`:
- Around line 318-359: A NullReferenceException can occur when a type has only
required properties, causing FirstOrDefault to return null for pd, and then
accessing pd.Name without validation crashes the code. In all three
files—src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpComplexObjectGenerator.cs
(lines 318-359),
src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpEntityGenerator.cs
(lines 398-440), and
src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude
(lines 69-95)—add a null check after the FirstOrDefault assignment to pd and
before the line that accesses pd.Name (string parameterName = pd.Name). If pd is
null, throw an InvalidOperationException with a descriptive message, following
the same pattern already implemented in DataContractProxyGenerator.cs.
In `@src/OpenRiaServices.Tools/Framework/DataContractProxyGenerator.cs`:
- Around line 214-269: Fix the grammar error in the exception message within the
GeneratePolyTypeDeserializationConstructor method. Change the error message
string from "All properties of {this.Type} is Required, cannot generate code" to
"All properties of {this.Type} are required; cannot generate code" to correct
the verb agreement (is to are), lowercase the property requirement adjective,
and improve punctuation consistency.
- Line 234: Replace the inconsistent string literal attribute declaration
pattern at line 234 in DataContractProxyGenerator.cs. Instead of passing a
string literal "PolyType.ConstructorShapeAttribute" directly to the
CodeAttributeDeclaration constructor in the
deserializingConstructor.CustomAttributes.Add call, use the established
CodeGenUtilities.CreateAttributeDeclaration method with the typeof operator
(following the same pattern used elsewhere in the file at lines 175, 186, and
466) to maintain consistency across all attribute declarations in this file.
🪄 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: 36824de5-9db7-4437-a997-700f576d54f7
📒 Files selected for processing (11)
NuGet/OpenRiaServices.Client.Core.nuspecsrc/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
Contributor
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/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/SerializationOptionsTests.cs (1)
207-267: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUnused local function
SerializeValue.The
SerializeValuehelper defined at lines 261-266 is never called withinBuildMessagePackInvokeRequest. This appears to be leftover code from a previous iteration.Proposed fix
using var ms = new MemoryStream(); serializer.SerializeObject(ms, payload, provider.GetTypeShapeOrThrow(payload.GetType())); var content = new ByteArrayContent(ms.ToArray()); content.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("application/vnd.msgpack"); return content; - - byte[] SerializeValue(object value, Type type) - { - using var valueStream = new MemoryStream(); - serializer.SerializeObject(valueStream, value, provider.GetTypeShapeOrThrow(type)); - return valueStream.ToArray(); - } }🤖 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.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/SerializationOptionsTests.cs` around lines 207 - 267, Remove the unused local function SerializeValue from within the BuildMessagePackInvokeRequest method, as it is never invoked anywhere in the method body. This leftover code from a previous iteration serves no purpose and should be deleted entirely.
🤖 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.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProvider.cs`:
- Around line 192-195: Fix the typo in the comment within the method that checks
metaMember.IsDataMember. Change the word "Similart" to "Similar" in the comment
that reads "Similart to SerializationUtility.IsSerializableDataMember."
---
Outside diff comments:
In
`@src/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/SerializationOptionsTests.cs`:
- Around line 207-267: Remove the unused local function SerializeValue from
within the BuildMessagePackInvokeRequest method, as it is never invoked anywhere
in the method body. This leftover code from a previous iteration serves no
purpose and should be deleted entirely.
🪄 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: f838584f-f597-4c89-af6d-438312c3f985
📒 Files selected for processing (6)
src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ChangeSetEntryConverter.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProvider.cssrc/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/MessagePack/FilteredTypeShapeProviderTests.cssrc/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/SerializationOptionsTests.cssrc/Test/AspNetCoreWebsite/Program.cs
💤 Files with no reviewable changes (1)
- src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cs
Contributor
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/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProviderOptions.cs (1)
3-14: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd XML docs for the new internal APIs.
AssociationMemberSerializationModeandFilteredTypeShapeProviderOptionsare newly introduced internal APIs and should be documented.Suggested fix
+/// <summary> +/// Controls whether association members are included when generating filtered type shapes. +/// </summary> enum AssociationMemberSerializationMode { + /// <summary>Exclude association members.</summary> Exclude = 0, + /// <summary>Include association members.</summary> Include = 1, } +/// <summary> +/// Options for <see cref="FilteredTypeShapeProvider"/>. +/// </summary> sealed class FilteredTypeShapeProviderOptions { + /// <summary> + /// Gets the default options instance. + /// </summary> public static FilteredTypeShapeProviderOptions Default { get; } = new(); + /// <summary> + /// Gets the association-member serialization mode. Defaults to <see cref="AssociationMemberSerializationMode.Exclude"/>. + /// </summary> public AssociationMemberSerializationMode AssociationMemberSerializationMode { get; init; } = AssociationMemberSerializationMode.Exclude; }As per coding guidelines, “New public, protected, and internal APIs should have XML documentation comments.”
🤖 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.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProviderOptions.cs` around lines 3 - 14, The AssociationMemberSerializationMode enum and FilteredTypeShapeProviderOptions class lack XML documentation comments despite being newly introduced internal APIs. Add XML documentation comments (using /// summary tags) to both the AssociationMemberSerializationMode enum and its members (Exclude and Include), and to the FilteredTypeShapeProviderOptions class and its Default property and AssociationMemberSerializationMode property. Each comment should clearly describe the purpose and usage of the API element it documents.Source: Coding guidelines
src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverter.cs (1)
17-19: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument
ObjectConverter<T>with XML comments.
ObjectConverter<T>is a new internal API and should include XML documentation to satisfy repository API-doc requirements.As per coding guidelines, "New public, protected, and internal APIs should have XML documentation comments".
🤖 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.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverter.cs` around lines 17 - 19, The ObjectConverter<T> class declaration is missing XML documentation comments required by the repository's API-doc standards for new internal APIs. Add XML documentation comments above the ObjectConverter<T> class declaration that describe the class's purpose, including a summary tag explaining what this converter does, to comply with the internal API documentation requirements.Source: Coding guidelines
src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/MessagePackMethodParametersConverter.cs (1)
8-12: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd XML docs for the new internal converter API.
The new internal API surface here is undocumented. Please add XML documentation on the converter type (and inherited summary on overrides where needed) to match repository rules.
As per coding guidelines, "New public, protected, and internal APIs should have XML documentation comments".
🤖 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.DomainClients.Http/Framework/MessagePack/Converters/MessagePackMethodParametersConverter.cs` around lines 8 - 12, Add XML documentation comments to the MessagePackMethodParametersConverter class and its members. Document the class itself with a summary explaining its purpose as an internal converter for MessagePack method parameters. Add inherited documentation summaries to the PreferAsyncSerialization property override and the Read method override to describe their roles in the conversion process. Ensure all documentation follows the repository's XML docs conventions for internal APIs.Source: Coding guidelines
♻️ Duplicate comments (1)
src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cs (1)
21-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix discriminator lookup to inspect the current base type.
At Line 25, the loop walks
baseTypebut still queries attributes fromtype, so attributes declared on base classes are never discovered.Suggested fix
- var derivedTypeShapes = type.GetCustomAttributes(typeof(PolyType.DerivedTypeShapeAttribute), inherit: false) + var derivedTypeShapes = baseType.GetCustomAttributes(typeof(PolyType.DerivedTypeShapeAttribute), inherit: false) .Cast<PolyType.DerivedTypeShapeAttribute>() .FirstOrDefault(p => p.Type == type);#!/bin/bash # Verify the reflection target used inside the base-type traversal loop. sed -n '19,36p' src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cs # Expected after fix: baseType.GetCustomAttributes(...), not type.GetCustomAttributes(...)🤖 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.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cs` around lines 21 - 27, The discriminator lookup in the while loop is querying attributes from the original type parameter instead of the baseType being inspected in each iteration. In the method that walks through base types starting at line 21, change the GetCustomAttributes call to query baseType instead of type so that attributes declared on actual base classes are discovered during the traversal.
🤖 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.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/MethodParametersConverter.cs`:
- Line 3: Remove the unused using directive for System.Reflection.Metadata from
the top of the MethodParametersConverter.cs file. This namespace is imported but
no types from it are actually referenced anywhere in the code, so the using
statement should be deleted to keep the imports clean.
In `@src/Test/AspNetCoreWebsite/MessagePack/BinaryConverter.cs`:
- Line 32: The call to CollectionsMarshal.AsSpan(value) at the writer.Write
invocation is incompatible with System.Data.Linq.Binary because
CollectionsMarshal.AsSpan only accepts List<T>. Replace the
CollectionsMarshal.AsSpan(value) call with value.ToArray() to properly convert
the Binary object to a byte array that can be written.
---
Outside diff comments:
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/MessagePackMethodParametersConverter.cs`:
- Around line 8-12: Add XML documentation comments to the
MessagePackMethodParametersConverter class and its members. Document the class
itself with a summary explaining its purpose as an internal converter for
MessagePack method parameters. Add inherited documentation summaries to the
PreferAsyncSerialization property override and the Read method override to
describe their roles in the conversion process. Ensure all documentation follows
the repository's XML docs conventions for internal APIs.
In
`@src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverter.cs`:
- Around line 17-19: The ObjectConverter<T> class declaration is missing XML
documentation comments required by the repository's API-doc standards for new
internal APIs. Add XML documentation comments above the ObjectConverter<T> class
declaration that describe the class's purpose, including a summary tag
explaining what this converter does, to comply with the internal API
documentation requirements.
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProviderOptions.cs`:
- Around line 3-14: The AssociationMemberSerializationMode enum and
FilteredTypeShapeProviderOptions class lack XML documentation comments despite
being newly introduced internal APIs. Add XML documentation comments (using ///
summary tags) to both the AssociationMemberSerializationMode enum and its
members (Exclude and Include), and to the FilteredTypeShapeProviderOptions class
and its Default property and AssociationMemberSerializationMode property. Each
comment should clearly describe the purpose and usage of the API element it
documents.
---
Duplicate comments:
In
`@src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cs`:
- Around line 21-27: The discriminator lookup in the while loop is querying
attributes from the original type parameter instead of the baseType being
inspected in each iteration. In the method that walks through base types
starting at line 21, change the GetCustomAttributes call to query baseType
instead of type so that attributes declared on actual base classes are
discovered during the traversal.
🪄 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: 5ada93ee-e5e9-429c-9af7-1996ab253986
📒 Files selected for processing (23)
NuGet/OpenRiaServices.Client.Core.nuspecsrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/MessagePackMethodParametersConverter.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverter.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Envelopes.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/MessagePackHttpDomainClient.cssrc/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/InvokeOperationInvoker.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/QueryOperationInvoker.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/DataContractRequestSerializer.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/MethodParametersConverter.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/SurrogateConverter.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProvider.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProviderOptions.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackRequestSerializer.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackResponseEnvelopeBase.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cssrc/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/SerializationProvider.cssrc/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DataContractSurrogateGenerator.cssrc/OpenRiaServices.Server/Framework/Data/DomainService.cssrc/Test/AspNetCoreWebsite/MessagePack/BinaryConverter.cssrc/Test/AspNetCoreWebsite/MessagePack/XElementConverter.cssrc/Test/AspNetCoreWebsite/Program.cssrc/Test/AspNetCoreWebsite/Properties/launchSettings.json
💤 Files with no reviewable changes (2)
- src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Envelopes.cs
- src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackResponseEnvelopeBase.cs
| return; | ||
| } | ||
|
|
||
| writer.Write(CollectionsMarshal.AsSpan(value)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET 10 API docs, what overloads exist for System.Runtime.InteropServices.CollectionsMarshal.AsSpan, and does any overload accept System.Data.Linq.Binary?
💡 Result:
In.NET 10, the System.Runtime.InteropServices.CollectionsMarshal.AsSpan method has only one overload, which is generic: public static Span AsSpan(List? list) This method provides a Span view over the data in a System.Collections.Generic.List [1][2][3]. There is no overload of CollectionsMarshal.AsSpan that accepts System.Data.Linq.Binary [1][4]. CollectionsMarshal is intended to provide unsafe access to the underlying data structures of specific collection types (such as List), and System.Data.Linq.Binary is not among the supported types [1][4].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.collectionsmarshal?view=net-10.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.collectionsmarshal.asspan?view=net-10.0
- 3: https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5ac14/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CollectionsMarshal.cs
- 4: https://learn.microsoft.com/is-is/dotnet/api/system.runtime.interopservices.collectionsmarshal?view=net-10.0&viewFallbackFrom=netframework-4.8
🏁 Script executed:
# First, let's find and examine the BinaryConverter.cs file
cat -n src/Test/AspNetCoreWebsite/MessagePack/BinaryConverter.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 1544
🌐 Web query:
System.Data.Linq.Binary class methods and properties, does it have ToArray method
💡 Result:
Yes, the System.Data.Linq.Binary class does have a ToArray method [1][2]. This method returns a byte array (byte[]) that contains the value of the current binary object [2][3]. In addition to ToArray, the Binary class includes other members such as: - Constructor: Binary(Byte[]) – Initializes a new instance of the Binary class [1][4]. - Properties: Length – Gets the length of the binary object [1]. - Methods: - Equals – Determines whether two binary objects are equal [1]. - GetHashCode – Serves as a hash function for the type [1][5]. - ToString – Returns a string representation of the binary object [1][6]. - Operators: Implicit(Byte[] to Binary) – Enables byte arrays to be implicitly coerced to the Binary type [1][7].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.data.linq.binary?view=netframework-4.8.1
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.data.linq.binary.toarray?view=netframework-4.8.1
- 3: https://learn.microsoft.com/en-us/previous-versions/windows/silverlight/dotnet-windows-silverlight/bb354903(v=vs.95)
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.data.linq.binary.-ctor?view=netframework-4.8.1
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.data.linq.binary.gethashcode?view=netframework-4.8.1
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.data.linq.binary.tostring?view=netframework-4.8.1
- 7: https://learn.microsoft.com/en-us/dotnet/api/system.data.linq.binary.op_implicit?view=netframework-4.8.1
CollectionsMarshal.AsSpan does not accept System.Data.Linq.Binary and will cause a compilation error.
Line 32 uses an API overload that only supports List<T>, not Binary. Use value.ToArray() instead, which is the correct method for converting Binary to a byte array.
Proposed fix
- writer.Write(CollectionsMarshal.AsSpan(value));
+ writer.Write(value.ToArray());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| writer.Write(CollectionsMarshal.AsSpan(value)); | |
| writer.Write(value.ToArray()); |
🧰 Tools
🪛 OpenGrep (1.22.0)
[WARNING] 32-32: Response.Write() with dynamic content can lead to XSS. Use HTML encoding or Razor syntax with automatic escaping instead.
(coderabbit.xss.csharp-response-write)
🤖 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/AspNetCoreWebsite/MessagePack/BinaryConverter.cs` at line 32, The
call to CollectionsMarshal.AsSpan(value) at the writer.Write invocation is
incompatible with System.Data.Linq.Binary because CollectionsMarshal.AsSpan only
accepts List<T>. Replace the CollectionsMarshal.AsSpan(value) call with
value.ToArray() to properly convert the Binary object to a byte array that can
be written.
Member
Author
There was a problem hiding this comment.
Binary derives from List so it can be implicitly cast to a list and should work
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
System.Data.Linq.Binary class definition inheritance hierarchy sealed class base type dotnet source
Length of output: 16245
@Daniel-Svensson, I've verified against the reference source and the official docs — System.Data.Linq.Binary is defined as:
public sealed class Binary : IEquatable<Binary>Its inheritance chain is Object → Binary only. It stores data internally as a byte[] array, not a List<byte>, so there's no implicit cast to List<byte> that would satisfy CollectionsMarshal.AsSpan<T>(List<T>?). The only implicit conversion defined on Binary goes the other direction: byte[] → Binary.
The value.ToArray() fix remains necessary here.
🧠 Learnings used
Learnt from: Daniel-Svensson
Repo: OpenRIAServices/OpenRiaServices PR: 512
File: src/OpenRiaServices.Server/Test/DomainServiceDescriptionTest.cs:385-387
Timestamp: 2026-03-08T23:08:53.507Z
Learning: In OpenRiaServices code reviews, ensure that the AssociationTypeDescriptor compatibility wrapper in DomainTypeDescriptionProvider.GetTypeDescriptor is applied only when there is a custom TypeDescriptionProvider and it does NOT originate from an OpenRiaServices assembly with version 5.7.0 or later. For OpenRiaServices >= 5.7.0, entities already emit EntityAssociationAttribute and must not be double-wrapped. This guideline applies to C# source files under src (e.g., any DomainTypeDescriptionProvider usage checks, tests, or related providers).
Learnt from: Daniel-Svensson
Repo: OpenRIAServices/OpenRiaServices PR: 591
File: src/Test/AspNetCoreWebsite/MessagePack/CustomComparerProvider.cs:6-20
Timestamp: 2026-06-15T14:23:04.067Z
Learning: In OpenRIAServices/OpenRiaServices, the “require XML documentation comments for new public, protected, and internal APIs” guideline should not be enforced for test projects. For any C# file under `src/Test/` (including test website projects like `src/Test/AspNetCoreWebsite/`), do not flag missing XML documentation comments on newly added `public`, `protected`, or `internal` members.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Daniel-Svensson
commented
Jun 15, 2026
12 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces MessagePack (application/vnd.msgpack) as a new wire format for OpenRiaServices over ASP.NET Core HTTP, including server-side serialization providers, client-side HTTP domain client support, and associated codegen/test/doc updates.
Changes:
- Added MessagePack serialization provider and request/response envelope handling in
OpenRiaServices.Hosting.AspNetCore, plus newAddMessagePackSerialization()builder APIs. - Added MessagePack-capable HTTP domain client factory/client and supporting converters on the client side.
- Updated code generation baselines and tooling to add a PolyType “deserialization constructor” workaround for required members; added/updated tests and documentation.
Reviewed changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/CustomMethodTests.cs | Adjusts expected failure behavior for nonexistent entity action under MessagePack conditional builds |
| src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/ComplexTypeTests.cs | Improves assertion message clarity |
| src/Test/OpenRiaservices.EndToEnd.AspNetCore.Test/OpenRiaservices.EndToEnd.AspNetCore.Test.csproj | Links MessagePack helper code from test website into test project (with exclusions) |
| src/Test/OpenRiaservices.EndToEnd.AspNetCore.Test/Main.cs | Switches test client factory selection to MessagePack under NET10 |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/TestProvider_Scenarios_CodeGen.g.vb | Baseline update for new PolyType deserialization constructor |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/FullTypeNames/Scenarios/TestProvider_Scenarios_CodeGen.g.cs | Baseline update for new PolyType deserialization constructor |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/TestProvider_Scenarios_CodeGen.g.vb | Baseline update for new PolyType deserialization constructor |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Scenarios/TestProvider_Scenarios_CodeGen.g.cs | Baseline update for new PolyType deserialization constructor |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Mocks/MockCustomers.g.vb | Baseline update for new PolyType deserialization constructor |
| src/Test/Desktop/OpenRiaServices.Common.DomainServices.Test/Baselines/Default/Mocks/MockCustomers.g.cs | Baseline update for new PolyType deserialization constructor |
| src/Test/AspNetCoreWebsite/Properties/launchSettings.json | Reorders settings / adjusts ports for test website |
| src/Test/AspNetCoreWebsite/Program.cs | Enables MessagePack serialization in the ASP.NET Core test website |
| src/Test/AspNetCoreWebsite/MessagePack/XElementConverter.cs | Adds MessagePack converter for XElement |
| src/Test/AspNetCoreWebsite/MessagePack/CustomComparerProvider.cs | Adds custom comparer provider for MessagePack serialization |
| src/Test/AspNetCoreWebsite/MessagePack/BinaryConverter.cs | Adds MessagePack converter for System.Data.Linq.Binary |
| src/OpenRiaServices.Tools/Framework/DataContractProxyGenerator.cs | Adds PolyType constructor-shape deserialization constructor generation in CodeDOM generator |
| src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/Templates/DataContractGeneratorTemplate.ttinclude | Adds PolyType constructor-shape constructor generation to T4 template |
| src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpEntityGenerator.cs | Regenerates compiled T4 output including new ctor generation logic |
| src/OpenRiaServices.Tools.TextTemplate/Framework/CSharpGenerators/CSharpComplexObjectGenerator.cs | Regenerates compiled T4 output including new ctor generation logic |
| src/OpenRiaServices.Server/Test/DomainMethodServerTest.cs | Updates tests for stricter argument validation helpers |
| src/OpenRiaServices.Server/Framework/Data/KnownTypeUtilities.cs | Switches known type collection to HashSet and simplifies unioning behavior |
| src/OpenRiaServices.Server/Framework/Data/DomainServiceDescription.cs | Adds GetCustomMethodOrThrow and refactors known-type closure computation |
| src/OpenRiaServices.Server/Framework/Data/DomainService.cs | Uses GetCustomMethodOrThrow during operation resolution |
| src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cs | Exposes surrogate helpers/factories for MessagePack integration |
| src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DataContractSurrogateGenerator.cs | Adds ASP.NET Core-specific surrogate constructor generation path |
| src/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/SerializationOptionsTests.cs | Adds basic MessagePack roundtrip and builder chaining tests |
| src/OpenRiaServices.Hosting.AspNetCore/Test/OpenRiaServices.Hosting.AspNetCore.Test/Serialization/MessagePack/FilteredTypeShapeProviderTests.cs | Adds tests around filtered shape provider behavior and cycles |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/README.md | Documents enabling MessagePack wire format |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/OpenRiaServices.Hosting.AspNetCore.csproj | Adds net10 target and MessagePack + hashing package refs |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/SerializationProvider.cs | Splits serializer response-writing APIs into query/invoke/submit + error paths |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MimeTypes.cs | Adds MessagePack MIME constant |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePackSerializationProvider.cs | New MessagePack serialization provider |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePackSerializationOptions.cs | New MessagePack options type |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackUtility.cs | Shared MessagePack utilities (discriminator/comparers/config) |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackResponseEnvelopeBase.cs | Adds MessagePack envelope model types |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/MessagePackRequestSerializer.cs | Implements MessagePack request/response serialization |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProviderOptions.cs | Adds options controlling association member inclusion |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/FilteredTypeShapeProvider.cs | Implements filtered shape provider wrappers for MessagePack serialization |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/SurrogateConverter.cs | Adds server-side surrogate-based converter for entities/complex types |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/MethodParametersConverter.cs | Adds server-side converter for method parameter maps |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/MessagePack/Converters/ChangeSetEntryConverter.cs | Adds server-side ChangeSetEntry converter for submit payloads |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Serialization/DataContractRequestSerializer.cs | Updates DataContract serializer to new query/invoke/submit method split |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/SubmitOperationInvoker.cs | Uses submit-specific serializer API |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/QueryOperationInvoker.cs | Uses query-specific serializer API |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/Operations/InvokeOperationInvoker.cs | Uses invoke-specific serializer API |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/OpenRiaServicesOptionsBuilder.cs | Adds AddMessagePackSerialization() builder overloads |
| src/OpenRiaServices.Hosting.AspNetCore/Framework/AspNetCore/OpenRiaServicesConfigurationBuilder.cs | Fixes argument validation |
| src/OpenRiaServices.Client/Framework/OpenRiaServices.Client.csproj | Adds MessagePack dependency |
| src/OpenRiaServices.Client/Framework/Entity.cs | Adds MessagePack serialization callbacks integration |
| src/OpenRiaServices.Client/Framework/ComplexObject.cs | Adds MessagePack serialization callbacks integration |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/OpenRiaServices.Client.DomainClients.Http.csproj | Adds MessagePack + hashing deps and links shared utilities |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePackHttpDomainClientFactory.cs | Adds MessagePack client factory and serializer/converter setup |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/MessagePackMethodParameters.cs | Adds wrapper for parameter metadata + values |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/MessagePackHttpDomainClient.cs | Adds MessagePack HTTP domain client |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Envelopes.cs | Adds client-side envelope models |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ObjectConverter.cs | Adds discriminator-based “object” converter for known types |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/MessagePackMethodParametersConverter.cs | Adds client-side parameter serialization converter |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/MessagePack/Converters/ChangeSetEntryConverter.cs | Adds client-side ChangeSetEntry converter |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/Http/MetodParameters.cs | Seals MethodParameters |
| src/OpenRiaServices.Client.DomainClients.Http/Framework/Http/HttpDomainClient.cs | Exposes service interface type to derived clients |
| src/MessagePackWorkarounds.md | Adds draft notes on MessagePack limitations/workarounds |
| NuGet/OpenRiaServices.Client.Core.nuspec | Adds package dependencies for MessagePack + hashing |
| docs/messagepack-serialization.md | Adds draft MessagePack wire schema documentation |
Comment on lines
+7
to
+26
| Top-level object map: | ||
|
|
||
| - `parameters`: map<string, value?> | ||
| Parameter name to a MessagePack value serialized with the declared parameter type converter. `nil` means `null`. | ||
| - `queryOptions`: array of `ServiceQueryPart` (optional) | ||
| - `includeTotalCount`: bool (optional) | ||
|
|
||
| ## Success response envelope | ||
|
|
||
| Top-level object map: | ||
|
|
||
| - `result`: value? | ||
| MessagePack value for the declared operation return type. `nil` means `null` / no value. | ||
|
|
||
| ## Fault response envelope | ||
|
|
||
| Top-level object map: | ||
|
|
||
| - `fault`: `DomainServiceFault` | ||
|
|
|
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.
New Features
application/vnd.msgpackMIME typeAddMessagePackSerialization()ASP.NET Core builder methods for streamlined configurationDocumentation
Bug Fixes
Benchmarks
Query of 20 000 relativly small text heavy entities
Query of 50
Summary by CodeRabbit
Release Notes
application/vnd.msgpack) with request/response envelopes.AddMessagePackSerialization()for server configuration.