Skip to content

XmlSerializer dont skip date time offset #55101

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jul 15, 2021
Merged

XmlSerializer dont skip date time offset #55101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
93e2374
Add DateTimeOffset as a primitive for XmlSerializer.
StephenMolloy Jul 2, 2021
b573c46
Fix DefaultValueAttribute handling for DTO.
StephenMolloy Jul 2, 2021
4ba6726
Add tests for DateTimeAttribute and XmlSerializer.
StephenMolloy Jul 2, 2021
c902309
Missed a 'KnownType' spot.
StephenMolloy Jul 7, 2021
7becea3
Update SGen tests to use current dotnet.exe from build.
StephenMolloy Jul 10, 2021
6227ea2
Unbreak runtimeconfig creation for testing.
StephenMolloy Jul 10, 2021
e2e254c
Debugging failure to resolve CoreCLR path.
StephenMolloy Jul 10, 2021
e7b34cd
Test fixup.
StephenMolloy Jul 10, 2021
fa5b2cc
More test cleanup.
StephenMolloy Jul 10, 2021
b229d29
Temporarily disable generator tests.
StephenMolloy Jul 11, 2021
caabfe0
Use InitObj instead of constructors for DateTimeOffset and TimeSpan.
StephenMolloy Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...es/Microsoft.XmlSerializer.Generator/tests/Microsoft.XmlSerializer.Generator.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<DefineConstants>$(DefineConstants);XMLSERIALIZERGENERATORTESTS</DefineConstants>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<SkipTestsOnPlatform Condition="'$(TargetsMobile)' == 'true' or '$(TargetOS)' == 'FreeBSD' or '$(TargetArchitecture)' == 'arm' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'armel' or '$(TargetArchitecture)' == 'wasm'">true</SkipTestsOnPlatform>
<SkipTestsOnPlatform>true</SkipTestsOnPlatform>
</PropertyGroup>

<PropertyGroup>
Expand Down
13 changes: 13 additions & 0 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/CodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,19 @@ internal void Ldc(object o)
New(TimeSpan_ctor);
break;
}
else if (valueType == typeof(DateTimeOffset))
{
ConstructorInfo DateTimeOffset_ctor = typeof(DateTimeOffset).GetConstructor(
CodeGenerator.InstanceBindingFlags,
null,
new Type[] { typeof(long), typeof(TimeSpan) },
null
)!;
Ldc(((DateTimeOffset)o).Ticks); // ticks
Ldc(((DateTimeOffset)o).Offset); // offset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments seem a little redundant. Besides that, I don't understand what's going on here. If looks like it has an object in o of type DateTimeOffset, and it's emitting the IL to place this concrete value onto the stack and then create a new DateTimeOffset from it. But this doesn't make sense as we don't emit IL every time we serialize. So where does the DataTimeOffset object in o come from, and why do we statically emit IL for this single instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DefaultValueAttribute I believe. WritePrimitive->WriteCheckDefault->this code. When we're cobbling together the IL writer, if a serializable member has a default value attribute, then we need to add code to check against the default value, which may or may not be 'default(DateTimeOffset)'. This code gets hit for each unique default value. Ie, if all your DateTimeOffset fields have 12/31/1999 as their default, then we only hit this once to check against that one default value. But if one of your DateTimeOffset fields has 1/1/2000 as it's default unlike the others, then we hit this code again to check the default for that field.

New(DateTimeOffset_ctor);
break;
}
else
{
throw new NotSupportedException(SR.Format(SR.UnknownConstantType, valueType.AssemblyQualifiedName));
Expand Down
44 changes: 44 additions & 0 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/PrimitiveXmlSerializers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,18 @@ internal void Write_dateTime(object? o)
WriteElementStringRaw(@"dateTime", @"", FromDateTime(((System.DateTime)o)));
}

internal void Write_dateTimeOffset(object? o)
{
WriteStartDocument();
if (o == null)
{
WriteEmptyTag(@"dateTimeOffset", @"");
return;
}
DateTimeOffset dto = (DateTimeOffset)o;
WriteElementStringRaw(@"dateTimeOffset", @"", System.Xml.XmlConvert.ToString(dto));
}

internal void Write_unsignedByte(object? o)
{
WriteStartDocument();
Expand Down Expand Up @@ -454,6 +466,36 @@ internal sealed class XmlSerializationPrimitiveReader : System.Xml.Serialization
return (object?)o;
}

internal object? Read_dateTimeOffset()
{
object? o = null;
Reader.MoveToContent();
if (Reader.NodeType == System.Xml.XmlNodeType.Element)
{
if (((object)Reader.LocalName == (object)_id20_dateTimeOffset && (object)Reader.NamespaceURI == (object)_id2_Item))
{
if (Reader.IsEmptyElement)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you copied the pattern from TimeSpan. Do you know why these have an extra Reader.IsEmptyElement check? Is it to avoid breaking existing users where you are consuming from a version which output null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you noted, this method was copied from the same pattern used to handle TimeSpan. So I'm just guessing here. But it is worth noting that prior to this PR, XmlSerializer would emit xml elements for DateTimeOffset fields, but they would be empty. Without these check, we would start to fail noisily when updated readers see those empty elements, whereas previously those members were simply being restored with default values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is non-obvious. We need to do something about this. Either through documentation, or by letting it fail and have an AppContext switch to add this check. I'll let you decide which as it can be argued both ways whether we should be breaking people to alert them that they aren't getting the data they expect vs not introducing breaks and it's a philosophical more than technical question. But this behavior needs to be discoverable either through an exception or documentation. I'm going to approve this based on the presumption you will go the documentation route.

Reader.Skip();
o = default(DateTimeOffset);
}
else
{
o = System.Xml.XmlConvert.ToDateTimeOffset(Reader.ReadElementString());
}
}
else
{
throw CreateUnknownNodeException();
}
}
else
{
UnknownNode(null);
}
return (object?)o;
}

internal object? Read_unsignedByte()
{
object? o = null;
Expand Down Expand Up @@ -720,6 +762,7 @@ protected override void InitCallbacks()
private string _id15_unsignedLong = null!;
private string _id7_float = null!;
private string _id10_dateTime = null!;
private string _id20_dateTimeOffset = null!;
private string _id6_long = null!;
private string _id9_decimal = null!;
private string _id8_double = null!;
Expand All @@ -743,6 +786,7 @@ protected override void InitIDs()
_id15_unsignedLong = Reader.NameTable.Add(@"unsignedLong");
_id7_float = Reader.NameTable.Add(@"float");
_id10_dateTime = Reader.NameTable.Add(@"dateTime");
_id20_dateTimeOffset = Reader.NameTable.Add(@"dateTimeOffset");
_id6_long = Reader.NameTable.Add(@"long");
_id9_decimal = Reader.NameTable.Add(@"decimal");
_id8_double = Reader.NameTable.Add(@"double");
Expand Down
6 changes: 6 additions & 0 deletions ...aries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,11 @@ private void WriteMemberElementsIf(Member[] expectedMembers, Member? anyElementM
Reader.Skip();
value = default(TimeSpan);
}
else if (element.Mapping.TypeDesc!.Type == typeof(DateTimeOffset) && Reader.IsEmptyElement)
{
Reader.Skip();
value = default(DateTimeOffset);
}
else
{
if (element.Mapping.TypeDesc == QnameTypeDesc)
Expand Down Expand Up @@ -1219,6 +1224,7 @@ private object WritePrimitive(TypeMapping mapping, Func<object, string> readFunc
"Guid" => XmlConvert.ToGuid(value),
"Char" => XmlConvert.ToChar(value),
"TimeSpan" => XmlConvert.ToTimeSpan(value),
"DateTimeOffset" => XmlConvert.ToDateTimeOffset(value),
_ => throw new InvalidOperationException(SR.Format(SR.XmlInternalErrorDetails, $"unknown FormatterName: {mapping.TypeDesc.FormatterName}")),
};
return retObj;
Expand Down
1 change: 1 addition & 0 deletions ...aries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,7 @@ private string ConvertPrimitiveToString(object o, TypeDesc typeDesc)
"Guid" => XmlConvert.ToString((Guid)o),
"Char" => XmlConvert.ToString((char)o),
"TimeSpan" => XmlConvert.ToString((TimeSpan)o),
"DateTimeOffset" => XmlConvert.ToString((DateTimeOffset)o),
_ => o.ToString()!,
};
return stringValue;
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/Types.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ static TypeScope()
AddNonXsdPrimitive(typeof(Guid), "guid", UrtTypes.Namespace, "Guid", new XmlQualifiedName("string", XmlSchema.Namespace), new XmlSchemaFacet[] { guidPattern }, TypeFlags.CanBeAttributeValue | TypeFlags.CanBeElementValue | TypeFlags.XmlEncodingNotRequired | TypeFlags.IgnoreDefault);
AddNonXsdPrimitive(typeof(char), "char", UrtTypes.Namespace, "Char", new XmlQualifiedName("unsignedShort", XmlSchema.Namespace), Array.Empty<XmlSchemaFacet>(), TypeFlags.CanBeAttributeValue | TypeFlags.CanBeElementValue | TypeFlags.HasCustomFormatter | TypeFlags.IgnoreDefault);
AddNonXsdPrimitive(typeof(TimeSpan), "TimeSpan", UrtTypes.Namespace, "TimeSpan", new XmlQualifiedName("duration", XmlSchema.Namespace), Array.Empty<XmlSchemaFacet>(), TypeFlags.CanBeAttributeValue | TypeFlags.CanBeElementValue | TypeFlags.XmlEncodingNotRequired);
AddNonXsdPrimitive(typeof(DateTimeOffset), "dateTimeOffset", UrtTypes.Namespace, "DateTimeOffset", new XmlQualifiedName("dateTime", XmlSchema.Namespace), Array.Empty<XmlSchemaFacet>(), TypeFlags.CanBeAttributeValue | TypeFlags.CanBeElementValue | TypeFlags.XmlEncodingNotRequired);

AddSoapEncodedTypes(Soap.Encoding);

Expand Down Expand Up @@ -596,6 +597,8 @@ internal static bool IsKnownType(Type type)
return true;
else if (type == typeof(TimeSpan))
return true;
else if (type == typeof(DateTimeOffset))
return true;
else if (type == typeof(XmlNode[]))
return true;
break;
Expand Down
17 changes: 15 additions & 2 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ public abstract class XmlSerializationReader : XmlSerializationGeneratedCode
private string _charID = null!;
private string _guidID = null!;
private string _timeSpanID = null!;
private string _dateTimeOffsetID = null!;

protected abstract void InitIDs();

Expand Down Expand Up @@ -214,6 +215,7 @@ private void InitPrimitiveIDs()
_charID = _r.NameTable.Add("char");
_guidID = _r.NameTable.Add("guid");
_timeSpanID = _r.NameTable.Add("TimeSpan");
_dateTimeOffsetID = _r.NameTable.Add("dateTimeOffset");
_base64ID = _r.NameTable.Add("base64");

_anyURIID = _r.NameTable.Add("anyURI");
Expand Down Expand Up @@ -667,6 +669,8 @@ private byte[] ReadByteArray(bool isBase64)
value = new Guid(CollapseWhitespace(ReadStringValue()));
else if ((object)type.Name == (object)_timeSpanID)
value = XmlConvert.ToTimeSpan(ReadStringValue());
else if ((object)type.Name == (object)_dateTimeOffsetID)
value = XmlConvert.ToDateTimeOffset(ReadStringValue());
else
value = ReadXmlNodes(elementCanBeType);
}
Expand Down Expand Up @@ -764,6 +768,8 @@ private byte[] ReadByteArray(bool isBase64)
value = default(Nullable<Guid>);
else if ((object)type.Name == (object)_timeSpanID)
value = default(Nullable<TimeSpan>);
else if ((object)type.Name == (object)_dateTimeOffsetID)
value = default(Nullable<DateTimeOffset>);
else
value = null;
}
Expand Down Expand Up @@ -4704,13 +4710,20 @@ private void WriteElement(string source, string? arrayName, string? choiceSource
}
Writer.Indent++;

if (element.Mapping.TypeDesc!.Type == typeof(TimeSpan))
if (element.Mapping.TypeDesc!.Type == typeof(TimeSpan) || element.Mapping.TypeDesc!.Type == typeof(DateTimeOffset))
{
Writer.WriteLine("if (Reader.IsEmptyElement) {");
Writer.Indent++;
Writer.WriteLine("Reader.Skip();");
WriteSourceBegin(source);
Writer.Write("default(System.TimeSpan)");
if (element.Mapping.TypeDesc!.Type == typeof(TimeSpan))
{
Writer.Write("default(System.TimeSpan)");
}
else if (element.Mapping.TypeDesc!.Type == typeof(DateTimeOffset))
{
Writer.Write("default(System.DateTimeOffset)");
}
WriteSourceEnd(source);
Writer.WriteLine(";");
Writer.Indent--;
Expand Down
14 changes: 5 additions & 9 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3100,7 +3100,7 @@ private void WriteElement(string source, string? arrayName, string? choiceSource
{
}

if (element.Mapping.TypeDesc!.Type == typeof(TimeSpan))
if ((element.Mapping.TypeDesc!.Type == typeof(TimeSpan)) || element.Mapping.TypeDesc!.Type == typeof(DateTimeOffset))
{
MethodInfo XmlSerializationReader_get_Reader = typeof(XmlSerializationReader).GetMethod(
"get_Reader",
Expand All @@ -3125,14 +3125,10 @@ private void WriteElement(string source, string? arrayName, string? choiceSource
ilg.Ldarg(0);
ilg.Call(XmlSerializationReader_get_Reader);
ilg.Call(XmlReader_Skip);
ConstructorInfo TimeSpan_ctor = typeof(TimeSpan).GetConstructor(
CodeGenerator.InstanceBindingFlags,
null,
new Type[] { typeof(long) },
null
)!;
ilg.Ldc(default(TimeSpan).Ticks);
ilg.New(TimeSpan_ctor);
LocalBuilder tmpLoc = ilg.GetTempLocal(element.Mapping.TypeDesc!.Type);
ilg.Ldloca(tmpLoc);
ilg.InitObj(element.Mapping.TypeDesc!.Type);
ilg.Ldloc(tmpLoc);
WriteSourceEnd(source, element.Mapping.TypeDesc.Type);
ilg.Else();
WriteSourceBegin(source);
Expand Down
23 changes: 23 additions & 0 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,11 @@ private XmlQualifiedName GetPrimitiveTypeName(Type type)
typeName = "TimeSpan";
typeNs = UrtTypes.Namespace;
}
else if (type == typeof(DateTimeOffset))
{
typeName = "dateTimeOffset";
typeNs = UrtTypes.Namespace;
}
else if (type == typeof(XmlNode[]))
{
typeName = Soap.UrType;
Expand Down Expand Up @@ -345,6 +350,12 @@ protected void WriteTypedPrimitive(string? name, string? ns, object o, bool xsiT
type = "TimeSpan";
typeNs = UrtTypes.Namespace;
}
else if (t == typeof(DateTimeOffset))
{
value = XmlConvert.ToString((DateTimeOffset)o);
type = "dateTimeOffset";
typeNs = UrtTypes.Namespace;
}
else if (typeof(XmlNode[]).IsAssignableFrom(t))
{
if (name == null)
Expand Down Expand Up @@ -4321,6 +4332,18 @@ private void WriteValue(object value)
Writer.Write(((DateTime)value).Ticks.ToString(CultureInfo.InvariantCulture));
Writer.Write(")");
}
else if (type == typeof(DateTimeOffset))
{
Writer.Write(" new ");
Writer.Write(type.FullName);
Writer.Write("(");
Writer.Write(((DateTimeOffset)value).Ticks.ToString(CultureInfo.InvariantCulture));
Writer.Write(", new ");
Writer.Write(((DateTimeOffset)value).Offset.GetType().FullName);
Writer.Write("(");
Writer.Write(((DateTimeOffset)value).Offset.Ticks.ToString(CultureInfo.InvariantCulture));
Writer.Write("))");
}
else if (type == typeof(TimeSpan))
{
Writer.Write(" new ");
Expand Down
8 changes: 8 additions & 0 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,10 @@ private void SerializePrimitive(XmlWriter xmlWriter, object? o, XmlSerializerNam
{
writer.Write_TimeSpan(o);
}
else if (_primitiveType == typeof(DateTimeOffset))
{
writer.Write_dateTimeOffset(o);
}
else
{
throw new InvalidOperationException(SR.Format(SR.XmlUnxpectedType, _primitiveType!.FullName));
Expand Down Expand Up @@ -978,6 +982,10 @@ private void SerializePrimitive(XmlWriter xmlWriter, object? o, XmlSerializerNam
{
o = reader.Read_TimeSpan();
}
else if (_primitiveType == typeof(DateTimeOffset))
{
o = reader.Read_dateTimeOffset();
}
else
{
throw new InvalidOperationException(SR.Format(SR.XmlUnxpectedType, _primitiveType!.FullName));
Expand Down
78 changes: 78 additions & 0 deletions src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,84 @@ public static void Xml_DeserializeEmptyTimeSpanType()
}
}

[Fact]
public static void Xml_TypeWithDateTimeOffsetProperty()
{
var now = new DateTimeOffset(DateTime.Now);
var defDTO = default(DateTimeOffset);
var obj = new TypeWithDateTimeOffsetProperties { DTO = now };
var deserializedObj = SerializeAndDeserialize(obj,
@"<?xml version=""1.0""?>
<TypeWithDateTimeOffsetProperties xmlns:xsi=""http://www.w3.org/2001/XMLSchema-instance"" xmlns:xsd=""http://www.w3.org/2001/XMLSchema"">
<DTO>" + XmlConvert.ToString(now) + @"</DTO>
<DTO2>" + XmlConvert.ToString(defDTO) + @"</DTO2>
<NullableDTO xsi:nil=""true"" />
<NullableDefaultDTO xsi:nil=""true"" />
</TypeWithDateTimeOffsetProperties>");
Assert.StrictEqual(obj.DTO, deserializedObj.DTO);
Assert.StrictEqual(obj.DTO2, deserializedObj.DTO2);
Assert.StrictEqual(defDTO, deserializedObj.DTO2);
Assert.StrictEqual(obj.DTOWithDefault, deserializedObj.DTOWithDefault);
Assert.StrictEqual(defDTO, deserializedObj.DTOWithDefault);
Assert.StrictEqual(obj.NullableDTO, deserializedObj.NullableDTO);
Assert.True(deserializedObj.NullableDTO == null);
Assert.StrictEqual(obj.NullableDTOWithDefault, deserializedObj.NullableDTOWithDefault);
Assert.True(deserializedObj.NullableDTOWithDefault == null);
}

[Fact]
public static void Xml_DeserializeTypeWithEmptyDateTimeOffsetProperties()
{
//var def = DateTimeOffset.Parse("3/17/1977 5:00:01 PM -05:00"); // "1977-03-17T17:00:01-05:00"
var defDTO = default(DateTimeOffset);
string xml = @"<?xml version=""1.0""?>
<TypeWithDateTimeOffsetProperties xmlns:xsi=""http://www.w3.org/2001/XMLSchema-instance"" xmlns:xsd=""http://www.w3.org/2001/XMLSchema"">
<DTO />
<DTO2 />
<DTOWithDefault />
<NullableDefaultDTO />
</TypeWithDateTimeOffsetProperties>";
XmlSerializer serializer = new XmlSerializer(typeof(TypeWithDateTimeOffsetProperties));

using (StringReader reader = new StringReader(xml))
{
TypeWithDateTimeOffsetProperties deserializedObj = (TypeWithDateTimeOffsetProperties)serializer.Deserialize(reader);
Assert.NotNull(deserializedObj);
Assert.Equal(defDTO, deserializedObj.DTO);
Assert.Equal(defDTO, deserializedObj.DTO2);
Assert.Equal(defDTO, deserializedObj.DTOWithDefault);
Assert.True(deserializedObj.NullableDTO == null);
Assert.Equal(defDTO, deserializedObj.NullableDTOWithDefault);
}
}

[Fact]
public static void Xml_DeserializeDateTimeOffsetType()
{
var now = new DateTimeOffset(DateTime.Now);
string xml = @"<?xml version=""1.0""?><dateTimeOffset>" + now.ToString("o") + "</dateTimeOffset>";
XmlSerializer serializer = new XmlSerializer(typeof(DateTimeOffset));

using (StringReader reader = new StringReader(xml))
{
DateTimeOffset deserializedObj = (DateTimeOffset)serializer.Deserialize(reader);
Assert.Equal(now, deserializedObj);
}
}

[Fact]
public static void Xml_DeserializeEmptyDateTimeOffsetType()
{
string xml = @"<?xml version=""1.0""?><dateTimeOffset />";
XmlSerializer serializer = new XmlSerializer(typeof(DateTimeOffset));

using (StringReader reader = new StringReader(xml))
{
DateTimeOffset deserializedObj = (DateTimeOffset)serializer.Deserialize(reader);
Assert.Equal(default(DateTimeOffset), deserializedObj);
}
}

[Fact]
public static void Xml_TypeWithByteProperty()
{
Expand Down
Loading