Skip to content
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

ManagementDateTimeConverter.ToDateTime returns the wrong DateTimeKind #30128

Closed
mattjohnsonpint opened this issue Jul 3, 2019 · 3 comments · Fixed by #90149
Closed

ManagementDateTimeConverter.ToDateTime returns the wrong DateTimeKind #30128

mattjohnsonpint opened this issue Jul 3, 2019 · 3 comments · Fixed by #90149

Comments

@mattjohnsonpint
Copy link
Contributor

The System.Management.ManagementDateTimeConverter.ToDateTime method is setting DateTimeKind.Unspecified in the result here.

I believe this to be incorrect, as it explicitly adjusts the time to the local computer's offset. As a user of this API, I'd expect the resulting DateTime value's Kind property to be DateTimeKind.Local.

With unspecified kind, I'd expect the result to not have been adjusted to the local time zone.

Also, a couple of other observations about the ManagementDateTimeConverter class:

  • The fixes from May 2018 seen in the history don't appear to have been ported back to .NET Framework. For example, I can see that in the NetFX implementation it still uses TimeZone.CurrentTimeZone instead of TimeZoneInfo.Local.

  • It would seem prudent to make a ToDateTimeOffset method that retains the offset provided in the DMTF string. Otherwise the user has to go out of their way to parse the string again manually to obtain the original offset.

  • Likewise, it would seem prudent to have an overload of ToDMTFDateTime that accepts a DateTimeOffset. Otherwise, there's no capability to create a DMTF string that has anything other than the local offset or zero.

@mattjohnsonpint
Copy link
Contributor Author

@tarekgh

@tarekgh
Copy link
Member

tarekgh commented Jul 3, 2019

CC @Anipik the area owner.

@mj1856

The docs https://docs.microsoft.com/en-us/dotnet/api/system.management.managementdatetimeconverter.todatetime?view=netframework-4.8 agree with your that the returned DateTime should be in local time zone. I am wondering, does the DateTime reflect that in all its properties except the Kind property? or the the time is not really adjusted to local timezone at all?

The fixes from May 2018 seen in the history don't appear to have been ported back to .NET Framework.

We don't do that except if there is any strong reason. Considering the released 4.8 is the last version of the .NET, I wouldn't expect anything else can be ported there.

It would seem prudent to make a ToDateTimeOffset method that retains the offset provided in the DMTF string. Otherwise the user has to go out of their way to parse the string again manually to obtain the original offset.
Likewise, it would seem prudent to have an overload of ToDMTFDateTime that accepts a DateTimeOffset. Otherwise, there's no capability to create a DMTF string that has anything other than the local offset or zero.

If you have any new API suggestion, feel free to open new issue proposing the new APIs. follow the doc https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md for doing that.
Thanks for your report.

@mattjohnsonpint
Copy link
Contributor Author

I am wondering, does the DateTime reflect that in all its properties except the Kind property? or the the time is not really adjusted to local timezone at all?

Yes. All of the properties except the Kind are correct, as it manually subtracts the offset.

Now that I look at it again, there's also a subtle hidden bug here, in the call to TimeZoneInfo.Local.GetUtcOffset(datetime). The datetime passed in is constructed with the fields of the input value, so it's looking up the offset based on the wrong point in time. (The input value is not guaranteed to be in local time).

It would be simpler and more correct if the implementation simply constructed a DateTimeOffset from the input values, then returned its LocalDateTime property.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jul 28, 2020
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 8, 2023
Maximys pushed a commit to Maximys/runtime that referenced this issue Aug 11, 2023
Maximys pushed a commit to Maximys/runtime that referenced this issue Aug 11, 2023
Maximys pushed a commit to Maximys/runtime that referenced this issue Aug 14, 2023
…entDateTimeConverter.ToDateTime(string) of NetFX.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants