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

Guidance for resolving ambiguous DateTimes does not account for BaseUtcOffsetDelta #32773

Open
mattjohnsonpint opened this issue Nov 28, 2022 · 10 comments
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-fundamentals/svc Pri2

Comments

@mattjohnsonpint
Copy link
Contributor

In several places in the guidance for resolving ambiguous DateTime values (both in this doc and this doc), the example code uses the BaseUtcOffset property of the time zone to determine the offset for standard time. This unfortunately is a bug, because it does not take into account any BaseUtcOffsetDelta that may apply.

More about BaseUtcOffsetDelta in the blog post here:
https://devblogs.microsoft.com/dotnet/date-time-and-time-zone-enhancements-in-net-6/#timezoneinfo-adjustmentrule-improvements

Both GetUtcOffset and GetAmbiguousTimeOffsets will take BaseUtcOffset and BaseUtcOffsetDelta into account - the problem is only when using BaseUtcOffset by itself, because that assumes the BaseUtcOffsetDelta is zero.

Also - A much better way to resolve ambiguous and invalid DateTime values is to account for both with respect to a given time zone and resolve to a DateTimeOffset. I've used this approach for some time now with the following extension method on several StackOverflow answers (such as this one) and in real apps. Feel free to use it in revised guidance:

public static DateTimeOffset ToDateTimeOffset(this DateTime dt, TimeZoneInfo tz)
{
    if (dt.Kind != DateTimeKind.Unspecified)
    {
        // Handle UTC or Local kinds (regular and hidden 4th kind)
        DateTimeOffset dto = new DateTimeOffset(dt.ToUniversalTime(), TimeSpan.Zero);
        return TimeZoneInfo.ConvertTime(dto, tz);
    }

    if (tz.IsAmbiguousTime(dt))
    {
        // Prefer the daylight offset, because it comes first sequentially (1:30 ET becomes 1:30 EDT)
        TimeSpan[] offsets = tz.GetAmbiguousTimeOffsets(dt);
        TimeSpan offset = offsets[0] > offsets[1] ? offsets[0] : offsets[1];
        return new DateTimeOffset(dt, offset);
    }

    if (tz.IsInvalidTime(dt))
    {
        // Advance by the gap, and return with the daylight offset  (2:30 ET becomes 3:30 EDT)
        TimeSpan[] offsets = { tz.GetUtcOffset(dt.AddDays(-1)), tz.GetUtcOffset(dt.AddDays(1)) };
        TimeSpan gap = offsets[1] - offsets[0];
        return new DateTimeOffset(dt.Add(gap), offsets[1]);
    }

    // Simple case
    return new DateTimeOffset(dt, tz.GetUtcOffset(dt));
}

Document Details

Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

@adegeo
Copy link
Contributor

adegeo commented Nov 28, 2022

Thank you for pointing this out.

@tarekgh do you have any thoughts on this?

@adegeo adegeo added the doc-enhancement Improve the current content [org][type][category] label Nov 28, 2022
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Nov 28, 2022
@mattjohnsonpint
Copy link
Contributor Author

I'm also going to open an API proposal for this extension method, as I believe it should be built-in.

@adegeo
Copy link
Contributor

adegeo commented Nov 28, 2022

I'm also going to open an API proposal for this extension method, as I believe it should be built-in.

I think that's the https://github.com/dotnet/runtime repo

@mattjohnsonpint
Copy link
Contributor Author

Yes, but the docs should be updated in the meantime so users don't introduce bugs. I can send a PR if you agree.

@adegeo
Copy link
Contributor

adegeo commented Nov 28, 2022

We love PRs :) but @tarekgh should review, they're the subject matter expert.

@tarekgh
Copy link
Member

tarekgh commented Nov 28, 2022

@mattjohnsonpint what exactly the issue you are trying to help with here? Is it to give the choice for users to decide which time to get when having ambiguous time? and ensure getting some results when having invalid time?

@mattjohnsonpint
Copy link
Contributor Author

For these doc pages, the main issue is that the example code incorrectly uses BaseUtcOffset without accounting for BaseUtcOffsetDelta.

DateTime utcTime = DateTime.SpecifyKind(ambiguousTime - TimeZoneInfo.Local.BaseUtcOffset, DateTimeKind.Utc);

That is the bug in the example. The standard offset is the one that is the lesser of the two ambiguous offsets - but it is not always going to match BaseUtcOffset.

I'll open a separate issue in the runtime repo for the API proposal.

@mattjohnsonpint
Copy link
Contributor Author

It also shows TimeZoneInfo.Local there, which may or may not be applicable.

@mattjohnsonpint
Copy link
Contributor Author

The other page shows a similar bug, but only in the VB sample.

If offsets(ctr).Equals(TimeZoneInfo.Local.BaseUtcOffset) Then
    zoneDescription = TimeZoneInfo.Local.StandardName
Else
    zoneDescription = TimeZoneInfo.Local.DaylightName
End If

Again, there's no guarantee the BaseUtcOffset is the standard offset, because BaseUtcOffsetDelta may modify it.

@tarekgh
Copy link
Member

tarekgh commented Nov 28, 2022

Ok, thanks @mattjohnsonpint. submit the pr and I'll be happy to look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-fundamentals/svc Pri2
Projects
None yet
Development

No branches or pull requests

4 participants