-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[NRBF] Allow the users to decode System.Nullable<UserStruct> #118276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…WithMembersAndTypes in the payload
This reverts commit 3636ebe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables the NRBF decoder to handle the specific edge case where System.Nullable<UserStruct>
is serialized by BinaryFormatter as a SystemClass but the underlying user struct is serialized as a regular class record.
Key Changes:
- Adds support for
ClassWithMembersAndTypes
records when the binary type isSystemClass
- Includes comprehensive test coverage for the nullable user struct scenario
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs |
Adds ClassWithMembersAndTypes to allowed record types for SystemClass binary type to handle System.Nullable<UserStruct> |
src/libraries/System.Formats.Nrbf/tests/EdgeCaseTests.cs |
Adds test case and supporting types to verify nullable user struct deserialization works correctly |
Tagging subscribers to 'binaryformatter-migration': @adamsitnik, @bartonjs, @jeffhandley, @JeremyKuhne |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
/backport to release/9.0-staging |
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/16718808888 |
@adamsitnik an error occurred while backporting to "release/9.0-staging", please check the run log for details! Unexpected end of JSON input |
When
BinaryFormatter
serializes a type different than primitive type or array, it serializes some information about ever member (field). To be exact is serializes on byte that specifies the kind and additional information for specific kind (for example, a string thar represents the type name).This information is decoded here.
In this particular edge case, where the payload contains
System.Nullable<$UserStruct>
and it's null at least once, BF emits an information that it's aBinaryType.SystemClass
. When there are no nulls, it emitsBinaryType.Class
. In both cases, the actual record is persisted asClassWithMembersAndTypesRecord
, not**System**ClassWithMembersAndTypesRecord
. And so farNrbfDecoder
was rejecting such inputs, as they are simply invalid from the specification point of view (the record could beClassWithMembersAndTypesRecord
,ClassWithIdRecord
orNullRecord
).Since we can't update
BinaryFormatter
(it's no longer supported) and such payloads could be persisted somewhere, I believe that we should relax theNrbfDecoder
check.I have tried to make this check more restrictive (allow only for
System.Nullable<$UserType>
to be handled like this), but I can't guarantee that there is no other similar edge case. So the proposed fix is to always allowBinaryType.SystemClass
to be represented as non-system class record in the payload.This fix is going to unblock an important first party customer, so please keep in mind that I am going to try to backport it to 9.0.