Serialization of DataColumn Type Corrected #27286

Anipik · 2018-02-20T19:48:28Z

Fixes https://github.com/dotnet/corefx/issues/26472

ViktorHofer · 2018-02-20T19:57:33Z

src/System.Data.Common/src/System/Data/DataTable.cs


-                dc.DataType = (Type)info.GetValue(string.Format(formatProvider, "DataTable.DataColumn_{0}.DataType", i), typeof(Type));
+                string typeName = (string)info.GetValue(string.Format(formatProvider, "DataTable.DataColumn_{0}.DataType", i), typeof(string));
+                dc.DataType = Type.GetType(typeName);


We should throw here if the type isn't resolvable: Type.GetType(typeName, throwOnError: true);

@danmosemsft @stephentoub any strong opinion whether we should throw a TypeLoadException or a SerializationException here?

I think the common pattern is a SerializationException with a nested exception (TypeLoadException in this case)

ViktorHofer · 2018-02-20T19:59:51Z

src/System.Data.Common/tests/System/Data/DataTableTest.cs

+                ms.Seek(0, SeekOrigin.Begin);
+                dtDeserialized = (DataTable)bf.Deserialize(ms);
+            }
+            Assert.Equal(dc.DataType.ToString(), dtDeserialized.Columns[0].DataType.ToString());


Why do you need to call .ToString() on both types? I expect Assert.Equal to be able to check for equality of two types.

ViktorHofer · 2018-02-20T20:00:19Z

src/System.Data.Common/tests/System/Data/DataTableTest.cs

+        public void DataColumnTypeSerialization()
+        {
+            DataTable dt = new DataTable("MyTable");
+            DataColumn dc = new DataColumn("dc", typeof(Int32));


nit: int instead of Int32

danmoseley · 2018-02-20T20:48:40Z

src/System.Data.Common/src/System/Data/DataTable.cs

                info.AddValue(string.Format(formatProvider, "DataTable.DataColumn_{0}.MaxLength", i), Columns[i].MaxLength);
-                info.AddValue(string.Format(formatProvider, "DataTable.DataColumn_{0}.DataType", i), Columns[i].DataType);
-
+                info.AddValue(string.Format(formatProvider, "DataTable.DataColumn_{0}.DataType", i), Columns[i].DataType.AssemblyQualifiedName);


When someone tries to deserialize on .NET Core a DataTable serialized on NETFX using SerializationFormat.Binary, it will throw (because Type is not serializable). Is the error reasonable and if not can we improve it?

if I understand you correctly, you are talking about the situation when DataTable is serialized on netfx and when we are trying to deserialize it in .net core, it will throw an exception as DataTable.DataColumn_{0}.DataType will be of type Type rather than string.
We can add a private optional field to tell whether the data has been serialized on netfx or on netcore. Then we can take appropriate action while deserializing

I don't think you need a field. In .NET Framework it reads off the value here:

dc.SimpleType = (SimpleType)info.GetValue(string.Format(formatProvider, "DataTable.DataColumn_{0}.SimpleType", i), typeof(SimpleType));

In .NET Core you would check whether info contained a value for DataTable.DataColumn_{0}.SimpleType and if so then it came from .NET Framework and you can't deal with that so you have to bail with a SerializationException with a nice message explaining that .NET Core cannot deserialize DataTables sewrialized on .NET Framework using SerializationFormat.Binary and they should use SerializationFormat.Xml or some other serialization mechanism.

There is no way to check whether info contains string.Format(formatProvider, "DataTable.DataColumn_{0}.SimpleType", i), typeof(SimpleType) this or not

You can't browse through the SerializationInfo object (by using its internal helper methods) outside of System.Private.CoreLib. There's a reason for that.

I believe throwing a SerializationException is enough here. We don't do anything fancy at other places where we can't deserialize unsupported types.

Is there any reason we are not changing the identifier (DataTable.DataColumn_{0}.DataType)? We are now storing a different "kind" of data inside it, although for the same purpose. It seems to me it may be clearer to rename it.

Even if later we modified .NET Framework to read/write this "new" format, it does not require us use the same name.

yeah it would be good to change it . I suggestDataTable.DataColumn_{0}.DataType_AssemblyQualifiedName ?

@ViktorHofer

You can't browse through the SerializationInfo object (by using its internal helper methods) outside of System.Private.CoreLib.

I would have expected GetValueNoThrow to do the job? But it's fine as is.

Anipik · 2018-02-21T06:46:49Z

@dotnet-bot test OSX x64 Debug Build

Anipik · 2018-02-21T07:13:46Z

@dotnet-bot test Tizen armel Debug Build

Anipik · 2018-02-21T16:57:39Z

@danmosemsft any more feedback ?

Commit migrated from dotnet/corefx@a87573a

Serialization of dataType Corrected

99c975d

Anipik requested review from ViktorHofer and danmoseley February 20, 2018 19:48

ViktorHofer reviewed Feb 20, 2018

View reviewed changes

danmoseley reviewed Feb 20, 2018

View reviewed changes

Anipik added 2 commits February 20, 2018 13:31

throw exception if type not resolvable

b9b9ad1

Change Serialization info key

cbba925

danmoseley approved these changes Feb 21, 2018

View reviewed changes

Anipik merged commit a87573a into dotnet:master Feb 21, 2018

JeremyKuhne pushed a commit to JeremyKuhne/corefx that referenced this pull request Feb 21, 2018

Serialization of DataColumn Type Corrected (dotnet#27286)

4823787

karelz added this to the 2.1.0 milestone Mar 10, 2018

Anipik deleted the ColumnSerializer branch March 24, 2018 01:34

marek-safar mentioned this pull request Jan 31, 2020

DataTable serialisation is broken between 2.1 and previous versions or .NET framework dotnet/runtime#25707

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Serialization of DataColumn Type Corrected (dotnet/corefx#27286)

4a7d3c6

Commit migrated from dotnet/corefx@a87573a

Serialization of DataColumn Type Corrected #27286

Serialization of DataColumn Type Corrected #27286

Uh oh!

Conversation

Anipik commented Feb 20, 2018

Uh oh!

ViktorHofer Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Feb 21, 2018

Uh oh!

Anipik commented Feb 21, 2018

Uh oh!

Anipik commented Feb 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ViktorHofer Feb 20, 2018 •

edited

Loading

Anipik Feb 20, 2018 •

edited

Loading

ViktorHofer Feb 20, 2018 •

edited

Loading