DateTimeOffset - ToLocalTime().DateTime and ToUniversalTime().DateTime return wrong DateTimeKind

mrpmorris commented 2 months ago

Description

I have an instance of DateTimeOffset retrieve from an API. I now need to use this internally as a UTC DateTime to query against a SQL Server database column that has a type datetime2(7).

My problem is that the date.Kind is set to Unspecified, so it is assumed to be in local time, and then the value is adjusted for the query.

So I find myself having to do this...

DateTime fromUtc = DateTime.SpecifyKind(query.From.ToUniversalTime().DateTime, DateTimeKind.Utc);

Instead of this

DateTime fromUtc = query.From.ToUniversalTime().DateTime;

Which is something I cannot rely on other developers on the system remembering to do, and might not have reared its head if we hadn't got such excellent unit tests.....or the UK hadn't shifted into Daylight Savings this week :)

Reproduction Steps

See expected behaviour for unit tests.

Expected behavior

public class DateTimeOffsetTests
{
    [Fact]
    public void WhenConvertingToUniversalTime_ThenTheDateTimeShouldBeDateTimeKindUtc()
    {
        DateTimeOffset subject = DateTimeOffset.Parse("2024-04-01T12:34:56+10:00").ToUniversalTime();
        Assert.Equal(DateTimeKind.Utc, subject.DateTime.Kind);
    }

    [Fact]
    public void WhenConvertingToLocalTime_ThenTheDateTimeShouldBeDateTimeKindLocal()
    {
        DateTimeOffset subject = DateTimeOffset.Parse("2024-04-01T12:34:56+10:00").ToLocalTime();
        Assert.Equal(DateTimeKind.Local, subject.DateTime.Kind);
    }
}

Actual behavior

The DateTime returned always has Kind == DateTimeKind.Unspecified

Regression?

It seems it has always been this way.

Known Workarounds

DateTime.SpecifyKind

Configuration

No response

Other information

If this cannot be done due to it being a breaking change, may I recommend DateTimeOffset has two new properties

public DateTime LocalDateTime => ...
public DateTime UniversalDateTime => ...

These could set the DateTime.Kind appropriately, and then ToLocalTime() and ToUniversalTime() could (optionally) be marked as obsolete.

Clockwork-Muse commented 2 months ago

This was deliberate, to avoid the property being out-of-sync with the offset.
Note that:

In many cases the DateTime.Kind would end up being Unspecified anyways (since in most cases it's going to not be local or UTC)
An offset of 0 might not actually be UTC (this would be more obvious if there was an equivalent DateTimeZoned type).

If this cannot be done due to it being a breaking change, may I recommend DateTimeOffset has two new properties

Breaking change, yes, but these properties already exist, eg: DateTimeOffset.UtcDateTime

Which is something I cannot rely on other developers on the system remembering to do, and might not have reared its head if we hadn't got such excellent unit tests.....or the UK hadn't shifted into Daylight Savings this week :)

This implies your API server isn't running on UTC, which it should be (for safety reasons, as here).

mrpmorris commented 2 months ago

@Clockwork-Muse When I do ToUniversalTime or ToLocalTime it adjusts the offset automatically in the result.

As far as I know, if you are in +00:00 and convert to UTC then it is equivalent, so it doesn't matter which it returns.

My dev machine isn't running in UTC, you are correct.

Clockwork-Muse commented 2 months ago

When I do ToUniversalTime or ToLocalTime it adjusts the offset automatically in the result.

Sure, but those methods are intended to return DateTimeOffset, not DateTime. Instead of:

DateTime fromUtc = DateTime.SpecifyKind(query.From.ToUniversalTime().DateTime, DateTimeKind.Utc);

You only need:

DateTime fromUtc = query.From.UtcDateTime;

My dev machine isn't running in UTC, you are correct.

The timezone of your dev (or any other) machine should be irrelevant. If it's affecting test results in any way, you have bugs (in actual application, tests, or both) - API and other "server" applications should always explicitly specify the relevant timezone and culture for all operations (which aren't always UTC/invariant, if you're dealing with user-centric data), and not use the ambient machine/process culture.

The reason servers are traditionally set to UTC is for "safety" reasons, in case applications are not written correctly. (And also because historically computers didn't really understand the concept of timezones)

Enforcing the correct typing is something you're going to need to handle appropriately in your own org, likely with code reviews. Especially in server applications DateTimeKinds Local and Unspecified are almost never useful; outright banning DateTime in most of your application is potentially viable, other than whatever small glue layers are required (as here). If you're doing more involved date/time work (eg, a calendar/scheduling application), you will likely find NodaTime more ergonomic and useful/helpful.

mrpmorris commented 2 months ago

@Clockwork-Muse If DateTimeOffset is adjusting to UTC or Local time then its DateTime property should also reflect that.

Clockwork-Muse commented 2 months ago

As I said before, no, that's a major breaking change in behavior. (Anything relying on it is probably already subtly broken, but changing it would likely break even correctly implemented stuff)
Local in particular is problematic, because if the timezone changes for any reason there's now a mismatch between the offset and the assumption of "local". Note that this is made worse due to the fact that changes to the current timezone can come from any thread, so can be any part of your application can do this silently. Have fun with that!

From somebody who's thought a lot about date/time, DateTimeKind as a concept is fundamentally flawed, especially for server applications. Except when necessary for legacy APIs, you're better off treating DateTime as if the Kind is always Unspecified - but in most cases you should be using DateTimeOffset and relying on the offset (or using NodaTime if doing work with future dates/scheduling).

mrpmorris commented 2 months ago

I disagree, but I don't think arguing the point with you would be productive if you are not the person who gets to make the decision, no offence intended here at all.

I have presented my argument as to why it is not a good implementation, and also suggested a non-breaking alternative to solve the problem.

Clockwork-Muse commented 2 months ago

.... I've been trying to point out to you that your proposed non-breaking alternative is already implemented.

dotnet / runtime