Fixed deserialization issue reported in (#742) #1041
Conversation
| public void TestNullBasicTypeSerialization() | ||
| { | ||
| long? nullableLong = 11; | ||
| var serialized = Serialize<long?>(nullableLong); |
There was a problem hiding this comment.
it would be good to check what the actual bytes are here; maybe an out string hex from Serialize, and hex = BitConverter.ToString(stream.GetBuffer(), 0, (int)stream.Length); - and especially if we're talking about a change in behaviour from 2.4.6, we need to check that the bytes are the same as they were in 2.4.6; they probably are, but we need to prove it
There was a problem hiding this comment.
Tried to implement your proposal
src/protobuf-net.Core/Helpers.cs
Outdated
| @@ -32,39 +32,53 @@ internal static MethodInfo GetInstanceMethod(Type declaringType, string name, Ty | |||
| internal static bool IsSubclassOf(Type type, Type baseClass) | |||
| => type.IsSubclassOf(baseClass); | |||
|
|
|||
| public static ProtoTypeCode GetTypeCode(Type type) | |||
| public static ProtoTypeCode GetTypeCode(Type type, bool resolveNullableValueType = true) | |||
There was a problem hiding this comment.
I'm concerned with the default of this being true, since that means the default is to change the behaviour, making it harder for me to review the contexts in which this changes; IMO the default here should be false, then we can opt in at the places where we intend to change the behaviour.
At the moment, the PR only tells me "here's two places (L1236 and L2380) that behave exactly like they used to" - this is the uninteresting thing to me; what I'm interested in is the places that now behave differently.
Can we reverse this default? or make it non-default?
There was a problem hiding this comment.
Sure I will change it, makes perfect sense, I will change to false by default.
| @@ -126,7 +126,7 @@ internal static WireType GetWireType(TypeModel model, DataFormat format, Type ty | |||
| return format == DataFormat.Group ? WireType.StartGroup : WireType.String; | |||
| } | |||
|
|
|||
| switch (Helpers.GetTypeCode(type)) | |||
| switch (Helpers.GetTypeCode(type, true)) | |||
There was a problem hiding this comment.
interesting that this happens after the enum/model checks; this makes me wonder how <SomeEnum?> and <SomeValueTypeCustomModel?> behave - I'll need to look locally to try to assess the impact of this proposed change
@mgravell Hi Marc this is my attempt to fix #742, I have created the unit test which shows the fix is working, my doubt is if it messes up something else. Unit test says no, but I would really appreciate you taking a look.