Make DBNull serializable #13845

ViktorHofer · 2017-09-07T22:02:53Z

Relates to https://github.com/dotnet/corefx/issues/23398

danmoseley · 2017-09-07T22:07:40Z

src/mscorlib/shared/System/DBNull.cs

+
+        private DBNull(SerializationInfo info, StreamingContext context)
+        {
+            throw new NotSupportedException(SR.NotSupported_DBNullSerial);


need to add the string

oh, it's already there by mistake...

danmoseley · 2017-09-07T22:08:59Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+        {
+            if (info == null)
+                throw new ArgumentNullException(nameof(info));
+            Contract.EndContractBlock();


I don't think we're bothering with these

ghost · 2017-09-07T22:09:01Z

Please, trim it down. That's lot of unnecessary code.

ViktorHofer · 2017-09-07T22:09:35Z

I'm also for trimming it down. Thanks

danmoseley · 2017-09-07T22:10:48Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+            switch (_unityType)
+            {
+                case EmptyUnity:
+                    {


not sure the indents are right

ViktorHofer · 2017-09-07T22:12:16Z

I should have clarified that I didn't expect a code review but more the answer if I should trim it down. @danmosemsft please wait with commenting on code :)

danmoseley · 2017-09-07T22:13:53Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+#endif
+    class UnitySerializationHolder : ISerializable, IObjectReference
+    {
+        internal const int EmptyUnity = 0x0001;


We don't use any of these save NullUnity, and don't plan to use them either. If we remove all the others, a bunch of other code could be removed from this file, and it would still be compatible?

I would do that, would cut this file in half

Agreed, though it begs the question as to whether any have any short-term benefits in including, because of dependencies elsewhere that are blocked similarly to how the DBNull case blocks some. (Any longer term benefits can be decided on later).

ViktorHofer · 2017-09-07T22:45:16Z

That's the trimmed down version of it. We will need a manual typeforward for the UnitySerializationHolder here https://github.com/dotnet/corefx/blob/master/src/shims/manual/mscorlib.forwards.cs#L7 in corefx and tests here https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs#L239

And I'm not 100% sure if this really should remain in coreclr or just go into corefx near System.Data.Common.

JonHanna · 2017-09-07T22:50:38Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+
+            _unityType = info.GetInt32(UnityTypeName);
+            _data = info.GetString(DataName);
+            _assemblyName = info.GetString(AssemblyNameName);


This seems like more work than is necessary if looked at in isolation. Maybe a comment that it's for compatibility might help someone confused by that.

You could also remove the fields. There's no real need to read or store those values are written out for backward compat.

danmoseley · 2017-09-07T22:54:27Z

Seems DBNull has to stay in corelib because of various magic for it in Convert etc. So it would be tricky to move this new class up. Does not seem worth it.

ghost · 2017-09-07T22:56:06Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+#if CORECLR
+    internal
+#else
+    public  // On CoreRT, this must be public because of the Reflection.Core/CoreLib divide and the need to whitelist past the ReflectionBlock.


Now that this is only used for DBNull, it no longer needs to be public on CoreRT.

ghost · 2017-09-07T22:57:50Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+            _assemblyName = info.GetString(AssemblyNameName);
+        }
+
+        public virtual void GetObjectData(SerializationInfo info, StreamingContext context) =>


It looks weird for these methods to be marked "virtual" - makes it look like you're expecting someone to override them even though this class has no subtypes.

ViktorHofer · 2017-09-07T23:03:06Z

Now that this is only used for DBNull, it no longer needs to be public on CoreRT.

I wasn't sure about that but will believe you :)

ViktorHofer · 2017-09-07T23:05:10Z

What I'm still not sure is this:

Netfx code of UnitySerilizationHolder.

On serialization it first saves the data (string) https://referencesource.microsoft.com/#mscorlib/system/unityserializationholder.cs,137 and after that the UnityType (int).
But on deserialization it first reads the UnityType (int) https://referencesource.microsoft.com/#mscorlib/system/unityserializationholder.cs,173 and afterwards the data (string).

Does that make any sense? Am I missing something?

ghost · 2017-09-07T23:09:07Z

Now that this is only used for DBNull, it no longer needs to be public on CoreRT.

I wasn't sure about that but will believe you :)

Hmm, you have a point. We don't need it to be public for Reflection.Core's sake, but I guess the binary formatter would still need to be able to find the class via Reflection. So I guess we still need the ifdef - just edit the comment to delete the part about Reflection.Core.

ghost · 2017-09-07T23:14:51Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+        public object GetRealObject(StreamingContext context)
+        {
+            // We are always returning the same DBNull instance and ignoring serialization input.
+


If we're still trying to be compatible with legacy blobs, should we at least throw if "UnityType" isn't NullUnity?

We definitely should throw as otherwise users would just get a random object. But before I can do that I need to verify the weird behavior I posted above. Thanks!

The SerializationInfo isn't an ordered list - it's a dictionary of name-value pairs. So there's no problem.

JonHanna · 2017-09-07T23:18:19Z

But on deserialization it first reads the UnityType (int)

Ordering doesn't matter, so it was probably just whatever order for each seemed most convenient at the time.

ViktorHofer · 2017-09-07T23:26:46Z

Aaah yes I overlooked that. Seems I spent too much time with simple serializable types :D

ViktorHofer · 2017-09-07T23:27:57Z

I think it should be fine now. PTAL

ghost · 2017-09-07T23:32:33Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+        /// UnitySerializationHelper should return from a call to GetObjectData. It contains
+        /// the unityType (defined above) and any optional data (used only for the reflection types).
+        /// </summary>
+        public static void GetUnitySerializationInfo(SerializationInfo info, int unityType, string data, Assembly assembly)


GetUnitySerializationInfo can be made internal and the "data" and "assembly" parameters serve no purpose anymore.

Aside from that, LGTM.

forget my comment. was wrong.

Why wouldn't DBNull be able to access it? It lives in the same assembly as UnitySerializationHolder.

ViktorHofer · 2017-09-07T23:37:33Z

Thanks for the review.

@danmosemsft do you also want to review again? :)

danmoseley · 2017-09-07T23:55:57Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+            // We are only support deserializing DBNull and throwing for everything else.
+            if (_unityType != NullUnity)
+            {
+                throw new ArgumentException(SR.Argument_InvalidUnity);


Invalid Unity type. is not going to be a helpful message when deserialization fails of a perfectly good .NET Framework blob. How about Type '{0}' is not deserializable. where you get {0} from Data if it's avaialable else UnityType?

danmoseley · 2017-09-07T23:56:35Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

+{
+    /// <summary>
+    /// Holds Null class for which we guarantee that there is only ever one instance of.
+    /// This only exists for backwarts compatibility with 


backwarts is a new word .. and the sentence isn't complete

danmoseley

You'll want to run tests vs desktop blobs - including one that contains something like a serialized Assembly so it exercises the unexpected unity type path

danmoseley · 2017-09-08T18:00:06Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

@@ -53,7 +56,7 @@ public object GetRealObject(StreamingContext context)
            // We are only support deserializing DBNull and throwing for everything else.
            if (_unityType != NullUnity)
            {
-                throw new ArgumentException(SR.Argument_InvalidUnity);
+                throw new ArgumentException(SR.Format(SR.Argument_InvalidUnity, _data ?? "UnityType"));


throw new ArgumentException(SR.Format(SR.Argument_InvalidUnity, _data ?? _unityType ?? "UnityType"));

danmoseley · 2017-09-08T18:00:30Z

LGTM

danmoseley · 2017-09-08T18:01:17Z

It would be nice to get this into 2.0.2. But we may not have both sides in in time.

ViktorHofer · 2017-09-08T18:29:46Z

The corefx changes are already ready to push but I want to wait till my build is finished.

…lization Make DBNull serializable Signed-off-by: dotnet-bot <[email protected]>

Make DBNull serializable

ViktorHofer added the area-Meta-corelib label Sep 7, 2017

ViktorHofer self-assigned this Sep 7, 2017

ViktorHofer requested review from stephentoub and danmoseley September 7, 2017 22:02

danmoseley reviewed Sep 7, 2017

View reviewed changes

ViktorHofer force-pushed the DBNull-Serialization branch from 8aca216 to 6dd7394 Compare September 7, 2017 22:29

Make DBNull serializable

db966f6

dnfclas added the cla-already-signed label Sep 7, 2017

JonHanna reviewed Sep 7, 2017

View reviewed changes

ViktorHofer force-pushed the DBNull-Serialization branch from 6dd7394 to db966f6 Compare September 7, 2017 22:55

ghost reviewed Sep 7, 2017

View reviewed changes

PR feedback

f2706ae

More comments and remove of corert special case

29408dc

Revert access modifier change and update its comment

b142440

ghost reviewed Sep 7, 2017

View reviewed changes

Add exception handling for other input and add comments

35f8867

ghost reviewed Sep 7, 2017

View reviewed changes

Remove unused params

423d10d

danmoseley reviewed Sep 7, 2017

View reviewed changes

danmoseley approved these changes Sep 7, 2017

View reviewed changes

ViktorHofer added 2 commits September 8, 2017 16:56

PR feedback

b9a716a

Adding serializable attribute to type

d19b3ac

danmoseley reviewed Sep 8, 2017

View reviewed changes

ViktorHofer merged commit 07aa22b into dotnet:master Sep 8, 2017

ViktorHofer mentioned this pull request Sep 8, 2017

DbNull serialization with tests and forward dotnet/corefx#23897

Merged

dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Sep 9, 2017

Merge pull request dotnet/coreclr#13845 from ViktorHofer/DBNull-Seria…

8469690

…lization Make DBNull serializable Signed-off-by: dotnet-bot <[email protected]>

jkotas pushed a commit to dotnet/corert that referenced this pull request Sep 9, 2017

Merge pull request dotnet/coreclr#13845 from ViktorHofer/DBNull-Seria…

aabd265

…lization Make DBNull serializable Signed-off-by: dotnet-bot <[email protected]>

ViktorHofer deleted the DBNull-Serialization branch September 11, 2017 21:07

ViktorHofer added a commit to ViktorHofer/coreclr that referenced this pull request Sep 11, 2017

Merge pull request dotnet#13845 from ViktorHofer/DBNull-Serialization

76accdf

Make DBNull serializable

ViktorHofer mentioned this pull request Sep 11, 2017

release/2.0.0 - Make DBNull serializable #13906

Merged

Make DBNull serializable #13845

Make DBNull serializable #13845

Conversation

ViktorHofer commented Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 7, 2017

ViktorHofer commented Sep 7, 2017

Choose a reason for hiding this comment

ViktorHofer commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Sep 7, 2017

ViktorHofer commented Sep 7, 2017

ghost commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonHanna commented Sep 7, 2017

ViktorHofer commented Sep 7, 2017 • edited Loading

ViktorHofer commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Sep 7, 2017

danmoseley Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Sep 8, 2017

danmoseley commented Sep 8, 2017

ViktorHofer commented Sep 8, 2017

ViktorHofer commented Sep 7, 2017 •

edited

Loading

ViktorHofer commented Sep 7, 2017 •

edited

Loading

danmoseley Sep 7, 2017 •

edited

Loading