-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
XmlSerializer dont skip date time offset #55101
XmlSerializer dont skip date time offset #55101
Conversation
null | ||
)!; | ||
Ldc(((DateTimeOffset)o).Ticks); // ticks | ||
Ldc(((DateTimeOffset)o).Offset); // offset |
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.
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?
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.
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.
if (((object)Reader.LocalName == (object)_id20_dateTimeOffset && (object)Reader.NamespaceURI == (object)_id2_Item)) | ||
{ | ||
if (Reader.IsEmptyElement) | ||
{ |
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.
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?
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.
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.
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.
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.
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs
Outdated
Show resolved
Hide resolved
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.
DateTime and TimeSpan are value types so don't need to use new.
@StephenMolloy Was SkipTestsOnPlatform intentially set to true unconditionally in Microsoft.XmlSerializer.Generator.Tests.csproj? |
@drieseng, there was a chicken and egg problem. The changes in the Generator requires the changes in System.Xml.Serialization, but the tests are run against an older .NET 6 SDK that is installed as part of the restore step when first building the repo. The plan is once the build environment SDK is updated to include these changes, then the tests can be re-enabled. They were manually verified to pass with some copying around of built pieces to before committing so it wasn't committed blind. |
Add support for DateTimeOffset to XmlSerializer
Treat DateTimeOffset as a primitive XmlSerializer. We map this to a
derivation of the xsd 'datetime' datatype which we call 'dateTimeOffset.'
Treatment is similar to how we handle TimeSpan.
Fixes #1406