dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.77k stars 3.18k forks source link

DateTime.Kind comes back as Unspecified #4711

Closed gzak closed 2 years ago

gzak commented 8 years ago

If you store a DateTime object to the DB with a DateTimeKind of either Utc or Local, when you read that record back from the DB you'll get a DateTime object whose kind is Unspecified. Basically, it appears the Kind property isn't persisted. This means that a test to compare equality of the before/after value of a round trip to the DB will fail.

Is there something that needs to be configured for the Kind to be preserved? Or what's the recommended way to handle this?

rallets commented 3 years ago

@ChristopherHaws I can confirm that if I have a nullable DateTime I need an explicit DateTime? converter as posted by @gabynevada

JonPSmith commented 3 years ago

Hi @rallets,

Doesn't look like @ChristopherHaws is around. I can confirm that you don't need an explicit DateTime? converter. A null value (of any type) is just passed through as a null would calling your Value Converter. See this note at the in the EF Core docs on Value Converters.

I hope that helps.

jeremycook commented 3 years ago

@JonPSmith my experience matches @rallets ... Maybe there is some confusion over configuration for a nullable type vs. a null value being passed. I found that configuration is needed for both the nullable and non-nullable version of a DateTime if your model has properties of both types.

To clarify by example (and this was with EF Core 3 or 3.1, not sure if 5.0 behaves different) if I have a property like public DateTime? Created { get; set; } and I want to apply a type converter to it, the signature must match DateTime? and not simply DateTime. Perhaps this is a bug or somehow we are not using the type converter configuration API correctly!?

rallets commented 3 years ago

@JonPSmith Thanks, I see where the difference is. The link you are providing is about the built-in modelBuilder.[...].HasConversion(converter). In the snippet used to apply ~globally~ automatically the validators, there is a conditional check on the property type: if (property.ClrType == typeof(DateTime)) this means that if the type of the property is DateTime? the converter is not applied.

Hence in this case we need to handle the DateTime?, too:

if (property.ClrType == typeof(DateTime?)) {
    property.SetValueConverter(UtcNullableConverter);
}

Probably it's possible to make it working using only one converter (UtcConverter) for both types: if (property.ClrType == typeof(DateTime) || property.ClrType == typeof(DateTime?)) but I need to test it for confirmation.

JonPSmith commented 3 years ago

Yes @rallets, I didn't realise that you were referring to @gabynevada example which applies a value converter. Yes, changing the test would fix that.

Also see Arthur Vickers' comment in this issue. If anyone knows the right answer, then Arthur does!

joakimriedel commented 3 years ago

Probably it's possible to make it working using only one converter (UtcConverter) for both types: if (property.ClrType == typeof(DateTime) || property.ClrType == typeof(DateTime?)) but I need to test it for confirmation.

Just verified that this works in my codebase, using the same value converter for both nullable and non-nullable. Thanks for the tip @rallets !

ChristopherHaws commented 3 years ago

Thanks for confirming that bug in my snippet. I have updated the snippet to reflect the changes. 👍

peabnuts123 commented 3 years ago

Is it possible to wrap up some of the code-snippets posted above into some kind of annotation? Like

  [Required]
  [HasDateTimeKind(DateTimeKind.Utc)]
  public DateTime CreatedOn { get; set; }

My knowledge of customising EFCore's serialisation etc is only limited. If possible, maybe this could even land in something like System.ComponentModel.Annotations? Seems like there is a pretty wide use-case/need for people specifying the kind, as database providers to not encode this information.

jeremycook commented 3 years ago

@peabnuts123 That would be convenient. That attribute and the annotations folks graciously provided here still don't overcome the fact that you can still shoot yourself in the foot with DateTime...like when you instantiate a DateTime object without specifying its kind, or by specifying the wrong kind like when DateTime.Now is used when DateTime.UtcNow should have been. For folks who are growing tired of worrying about DateTime I suggest just using DateTimeOffset for now (no pun intended) and/or upvoting the Native support for NodaTime issue.

ChristopherHaws commented 3 years ago

@peabnuts123 Here is an updated version of my snippet that includes an attribute called IsUtc the behaves the same as the builder method:

public static class UtcDateAnnotation {
    private const string IsUtcAnnotation = "IsUtc";
    private static readonly ValueConverter<DateTime, DateTime> UtcConverter =
        new ValueConverter<DateTime, DateTime>(v => v, v => DateTime.SpecifyKind(v, DateTimeKind.Utc));

    public static PropertyBuilder<TProperty> IsUtc<TProperty>(this PropertyBuilder<TProperty> builder, bool isUtc = true) =>
        builder.HasAnnotation(IsUtcAnnotation, isUtc);

    public static bool IsUtc(this IMutableProperty property) {
        var attribute = property.PropertyInfo.GetCustomAttribute<IsUtcAttribute>();
        if (attribute is not null && attribute.IsUtc) {
            return true;
        }

        return ((bool?)property.FindAnnotation(IsUtcAnnotation)?.Value) ?? true;
    }

    /// <summary>
    /// Make sure this is called after configuring all your entities.
    /// </summary>
    public static void ApplyUtcDateTimeConverter(this ModelBuilder builder) {
        foreach (var entityType in builder.Model.GetEntityTypes()) {
            foreach (var property in entityType.GetProperties()) {
                if (!property.IsUtc()) {
                    continue;
                }

                if (property.ClrType == typeof(DateTime) ||
                    property.ClrType == typeof(DateTime?)) {
                    property.SetValueConverter(UtcConverter);
                }
            }
        }
    }
}

public class IsUtcAttribute : Attribute {
    public IsUtcAttribute(bool isUtc = true) => this.IsUtc = isUtc;
    public bool IsUtc { get; }
}

Something to note though, this snippet is not intended to "convert" the datetime into utc, but rather is used to specify the kind when reading the value from the database. You still need to be diligent and only ever pass in the correct type of DateTime. As far as I know it is not possible to write a converter that is smart enough to do the correct conversion logic since expressions are pretty limited. If it were possible though, DateTimeKind.Unspecified would probably be unhandled and should throw an exception since there is no way to know what timezone it is in.

DateTime functionality is weird (hence why NodaTime exists as @jeremycook pointed out)... The built in functionality makes a lot of assumptions that could be (and often are) wrong:

Taking on a dependency like NodaTime can be tricky too however because you need to add support to it throughout the entire stack of your app (i.e. EF Postgresql provider supports it, but the rest do not as far as I know, XML/JSON serializers need to be configured to use it, ASP.NET needs configuration, etc). Unfortunately there is no silver bullet here. What I do is either use DateTimes and always deal with things in UTC and let the view convert to the timezone the user wants to see things in or use DateTimeOffset, which has its own pros/cons.

peabnuts123 commented 3 years ago

That's top notch @ChristopherHaws. Thanks for the code and your dedication to this issue. That gets pretty much as close to an optimal solution as I think you can achieve without EF Core adopting it natively.

mikola-akbal commented 3 years ago

Offered is the best solution. Attribute or fluent API call. It will be convenient. Time in DB can be stored by the most economic way, but when EF Core takes it from DB and puts into C# property, it creates DateTime with assigned DateTimeKind. If it is not yet, it must be added to future functionality of EF Core.

[HasDateTimeKind(DateTimeKind.Utc)]
public DateTime CreatedOn { get; set; }

I think, keep time Kind in DB is not good idea, because it takes place, and necessity of it is very low. If you need it, you can keep additional Byte column in DB. Usually, time Kind is the same for all rows. So, it's more rational to keep it in EF Core entities configuration.

In my case, I simply convert all time from Unspecified to UTC, even without annotation. It seems, for global databases it's the most common case.

ChristopherHaws commented 3 years ago

@acbaile I am also storing all my dates as UTC in the database. The problem occurs when you read the dates out of the database. The default behavior is to read the values as "unspecified" instead of as UTC how they were stored. This means that if I we to call .ToUniversalTime() on them, I would now end up with the incorrect date. This gets particularly interesting when you further serialize the values into json or xml (as is my use case since the values are returned from an ASP.NET Core WebAPI). In this scenario the values would be read from the database as unspecified and then serialized to JSON without a 'Z' at the end to specify that it is zulu (UTC) time which would in turn be translated incorrectly by front-end libraries such as momentjs. Instead of constantly having to specify the kind on every datetime I read from the database, I wrote that snippet to do the work for me. :)

Here is a short snippet to show the problem:

void Main() {
    var utcNow = DateTime.UtcNow;

    var json = JsonConvert.SerializeObject(new {
        UtcDateTime = utcNow,
        UnspecifiedDateTime = DateTime.SpecifyKind(utcNow, DateTimeKind.Unspecified),
        UnspecifiedDateTimeOffset = new DateTimeOffset(DateTime.SpecifyKind(utcNow, DateTimeKind.Unspecified))
    }, Newtonsoft.Json.Formatting.Indented);

    Console.WriteLine(json);

    /*
    Outputs:
    {
      "UtcDateTime": "2020-11-30T09:05:44.3006035Z",
      "UnspecifiedDateTime": "2020-11-30T09:05:44.3006035",
      "UnspecifiedDateTimeOffset": "2020-11-30T09:05:44.3006035-08:00"
    }
    */
}

Hope that helps clarify!

mike-whitehurst-b2m commented 3 years ago

@ChristopherHaws Was there a reason for IsQueryType/IsKeyLess being in the earlier code sample? It results in no "Z" when reading from a View. Checking it's safe to remove.

ChristopherHaws commented 3 years ago

@mike-whitehurst-b2m I don't remember the exact reason since I have since removed that logic. I do know that the EF setup I use is slightly unconventional (I use mostly owned types using SQL provider) and there might have been a bug at the time that caused me to need that logic (I have found and reported several bugs related to owned types since I use them extensively). I would recommend you try out this snipped and make sure you add a few unit and integration tests that test your specific scenarios. Perhaps in the future I will try to package this snipped as a library, I was just hoping we would get support for this in EF Core directly. :)

fartwhif commented 2 years ago

five years and this still bites people in the ass, "closed", because the dogma has been established. It's incredible how much a pair or even a solitary integer can be the subject of so much waste for the sake of not wasting.

roji commented 2 years ago

@fartwhif EF Core already allows you to specify the DateTime.Kind coming back from the databases - via value converters - and 6.0 even allows you to configure that globally for all DateTime properties in your model. Beyond that it's hard to see what EF Core could do - SQL Server's datetime/datetime2 types simply do not store DateTimeKind, which is a .NET-only concept. Are you looking for anything in particular?

ChristopherHaws commented 2 years ago

@roji IMO, the only real solution is to store the value in two columns (datetime and timezone name). Unfortunately DateTimeOffset is not a real solution for this problem since many timezone's exist within a single offset (for example Arizona and Colorado are both in Mountain Time however Arizona does not have daylight savings while Colorado does) and those timezones could have different rules about daylight savings time. Since this isn't a feature of EF Core, I always store all my datetime's in UTC and manually store the timezone name when necessary. If only daylight savings didn't exist :P

roji commented 2 years ago

the only real solution is to store the value in two columns (datetime and timezone name)

Note that there's a very big difference between storing the DateTime's timestamp and kind, and storing a timestamp and timezone - this issue really is about fully persisting a DateTime, which does not have a timezone name (or even an offset).

Aside from that, it's not just the database that doesn't have a single type for timestamp+timezone, it's also .NET (I recommend looking at NodaTime, which does have ZonedDateTIme). Other than that, if your goal is to save a timestamp and timezone name, then you're right that you need two columns (see this blog post which I just recently wrote about the subject).

To summarize, on the .NET side you need two properties (DateTime and string, for the time zone), and on the SQL Server side you need two columns (datetime2 and nvarchar). If you want to use NodaTime (highly recommended), then on the .NET side you can have a single property of time ZonedDateTime. At the moment EF Core cannot map a single .NET property to multiple columns, but that feature is planned for EF7 (#13947).

SebFerraro commented 2 years ago

and 6.0 even allows you to configure that globally for all DateTime properties in your model

@roji could you elaborate on the global configuration? I cannot see to find a simple way of doing this in our new EF Core setup, that uses an existing schema (and therefore the two column option is not viable for us). We don't care about storing the datetime kind in the database, just that all datetime properties are returned as UTC instead of unspecified. At present our mapping code from ef model -> domain model custom converts every single property which means it's down to a developer to remember that...

roji commented 2 years ago

@SebFerraro above I just referred to the ability to configure value converters to modify the Kind on DateTimes being read, as here. That can be combined with the new EF Core 6.0 bulk configuration to apply this globally to your model.

But I want to stress again that this isn't the ideal way to do it. If what you want is to simply store UTC timestamps and to retrieve them as UTC DateTimes, use version 6.0 and map your EF DateTime properties to timestamp with time zone. This will automatically make DateTimes coming back have UTC Kind without any value converters.

SebFerraro commented 2 years ago

@SebFerraro above I just referred to the ability to configure value converters to modify the Kind on DateTimes being read, as here. That can be combined with the new EF Core 6.0 bulk configuration to apply this globally to your model.

But I want to stress again that this isn't the ideal way to do it. If what you want is to simply store UTC timestamps and to retrieve them as UTC DateTimes, use version 6.0 and map your EF DateTime properties to timestamp with time zone. This will automatically make DateTimes coming back have UTC Kind without any value converters.

@roji Thanks - I was more meaning that I can't see to find an example of the new convention specifically converting to datetime kind anywhere.

The individual method being:

modelBuilder.Entity<Post>()
    .Property(e => e.LastUpdated)
    .HasConversion(
        v => v,
        v => new DateTime(v.Ticks, DateTimeKind.Utc));

The format must have changed as I cannot figure out how to implement that at configbuilder level.

Eg. This does not work


            configurationBuilder
                .Properties<DateTime>()
                .HaveConversion(dt => dt, dt => new DateTime(dt.Ticks, DateTimeKind.Utc));

I also appreciate this is not the best way of doing it. But it's better than our current implementation working on a schema that's 14 years old with 160+ entities, all of which have 1-3 datetimes on them.

joakimriedel commented 2 years ago

@SebFerraro the bulk configuration using ConfigureConventions does not work with the new compiled models in EF Core 6, so we decided to adapt the code from @ChristopherHaws above slightly. Works like a charm!

using System;
using System.Reflection;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Builders;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;

namespace Whatever;

    public class UtcValueConverter : ValueConverter<DateTime, DateTime>
    {
        public UtcValueConverter()
            : base(v => v, v => DateTime.SpecifyKind(v, DateTimeKind.Utc))
        {
        }
    }

    public static class UtcDateAnnotation
    {
        private const string IsUtcAnnotation = "IsUtc";

        public static PropertyBuilder<TProperty> IsUtc<TProperty>(this PropertyBuilder<TProperty> builder, bool isUtc = true)
        {
            if (builder is null)
            {
                throw new ArgumentNullException(nameof(builder));
            }

            return builder.HasAnnotation(IsUtcAnnotation, isUtc);
        }

        public static bool IsUtc(this IMutableProperty property)
        {
            if (property is null)
            {
                throw new ArgumentNullException(nameof(property));
            }

            var attribute = property.PropertyInfo?.GetCustomAttribute<IsUtcAttribute>();
            if (attribute is not null && attribute.IsUtc)
            {
                return true;
            }

            return ((bool?)property.FindAnnotation(IsUtcAnnotation)?.Value) ?? true;
        }

        /// <summary>
        /// Make sure this is called after configuring all your entities.
        /// </summary>
        public static void ApplyUtcDateTimeConverter(this ModelBuilder builder)
        {
            if (builder is null)
            {
                throw new ArgumentNullException(nameof(builder));
            }

            foreach (var entityType in builder.Model.GetEntityTypes())
            {
                foreach (var property in entityType.GetProperties())
                {
                    if (!property.IsUtc())
                    {
                        continue;
                    }

                    if (property.ClrType == typeof(DateTime) ||
                        property.ClrType == typeof(DateTime?))
                    {
                        property.SetValueConverter(typeof(UtcValueConverter));
                    }
                }
            }
        }
    }

    [AttributeUsage(AttributeTargets.Property)]
    public class IsUtcAttribute : Attribute
    {
        public IsUtcAttribute(bool isUtc = true) => this.IsUtc = isUtc;
        public bool IsUtc { get; }
    }

then in THE END OF (important!) OnModelCreating just put

            // all entity configuration

            // note: this line has to be last in this method to work!

            builder.ApplyUtcDateTimeConverter();

so your developers only need to remember never put any configuration below that line, and all properties will be automatically converted for DateTimeKind.Utc on read.

@roji slightly off topic: the links in the first paragraph of the docs you sent a link for are broken (see "xref:") https://docs.microsoft.com/en-us/ef/core/modeling/bulk-configuration#pre-convention-configuration

EDIT: @SebFerraro did not see that you used existing schema, sorry. For others reading this issue with code-first and wanting to use precompiled model, I'll keep this post nevertheless...

SebFerraro commented 2 years ago

@joakimriedel it was an existing database that I ported into a brand new ef core project, and your solution has actually worked, which is great! Thanks!

roji commented 2 years ago

@joakimriedel @SebFerraro if what you want is for DateTime to always come back as UTC, there's no need for an attribute; @SebFerraro your code didn't work since HaveConversion in ConfigureConventions only accepts a value converter type, rather than lambdas (in order to better support compiled models). See below for a minimal code sample which shows everything working without an attribute.

Code sample ```c# await using var ctx = new BlogContext(); await ctx.Database.EnsureDeletedAsync(); await ctx.Database.EnsureCreatedAsync(); var blog = await ctx.Blogs.SingleAsync(); Console.WriteLine(blog.DateTime.Kind); public class BlogContext : DbContext { public DbSet Blogs { get; set; } protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) => optionsBuilder .UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0") .LogTo(Console.WriteLine, LogLevel.Information) .EnableSensitiveDataLogging(); protected override void OnModelCreating(ModelBuilder modelBuilder) => modelBuilder.Entity().HasData(new Blog { Id = 1, DateTime = DateTime.Now }); protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder) { configurationBuilder .Properties() .HaveConversion(typeof(UtcValueConverter)); } class UtcValueConverter : ValueConverter { public UtcValueConverter() : base(v => v, v => DateTime.SpecifyKind(v, DateTimeKind.Utc)) { } } } public class Blog { public int Id { get; set; } public DateTime DateTime { get; set; } } ```

@roji slightly off topic: the links in the first paragraph of the docs you sent a link for are broken (see "xref:") https://docs.microsoft.com/en-us/ef/core/modeling/bulk-configuration#pre-convention-configuration

Thanks. The 6.0 API docs (which this links point to) have only recently been pushed online in https://github.com/dotnet/EntityFramework.Docs/issues/3630, and I don't think we've pushed our conceptual docs since then - this should go away the next time we do.

joakimriedel commented 2 years ago

@roji the attribute is a negative option, so the code will actually set the value converter for all DateTime properties without using any attributes.

But regarding the ConfigureConventions, I actually never tried it since the method has the following remark:

If a model is explicitly set on the options for this context (via Microsoft.EntityFrameworkCore.DbContextOptionsBuilder.UseModel(Microsoft.EntityFrameworkCore.Metadata.IModel)) then this method will not be run.

Since I explicitly set a model via UseModel I never even bothered to try it.

But you're saying it should work with compiled models? I will try right away!

EDIT: It actually works. 🤯 I'd definitely suggest rewording this remark, it had me fooled ever since 6.0 released. Perhaps

If a model is explicitly set on the options for this context (via Microsoft.EntityFrameworkCore.DbContextOptionsBuilder.UseModel(Microsoft.EntityFrameworkCore.Metadata.IModel)) then this method will only run while compiling the model.

.. or something similar, if this is actually how it works?

roji commented 2 years ago

/cc @AndriySvyryd for the above remark

qsdfplkj commented 2 years ago

In order to save it as Utc I use:

builder.Property(x => x.DateTime).HasConversion((x) => x.ToUniversalTime(), (x) => DateTime.SpecifyKind(x, DateTimeKind.Utc) );

This way if someone uses unspecified or local time at least it is converted to utc time.

ChristopherHaws commented 2 years ago

@qsdfplkj Be carful with this approach. If the DateTimeKind is Unspecified and you call ToUniversalTime, it will assume that the DateTime is in local time which can cause some hard to find bugs. This can easily happen when crossing over a serialization boundary such as a WebAPI.

qsdfplkj commented 2 years ago

Thanks I will check this serialization scenario.

palhal commented 2 years ago

Here's my take on this for a nullable DateTime, inspired by @qsdfplkj's and @ChristopherHaws' answers:

public DateTime? TimestampUtc { get; set; }
builder.Property(x => x.TimestampUtc)
    .HasConversion(new UtcDateTimeConverter());
public class UtcDateTimeConverter : ValueConverter<DateTime?, DateTime?>
{
    public UtcDateTimeConverter()
        : base(
            d => !d.HasValue ? null : ConvertToUtc(d.Value),
            d => !d.HasValue ? null : SpecifyUtc(d.Value))
    {
    }

    private static DateTime ConvertToUtc(DateTime date)
    {
        switch (date.Kind)
        {
        case DateTimeKind.Utc:
            return date;
        case DateTimeKind.Local:
            return date.ToUniversalTime();
        default:
            throw new InvalidTimeZoneException($"Unsupported DateTimeKind: {date.Kind}");
        }
    }

    private static DateTime SpecifyUtc(DateTime date)
    {
        return DateTime.SpecifyKind(date, DateTimeKind.Utc);
    }
}
qsdfplkj commented 2 years ago

I didn't use this approach anymore because usually data comes in from an API and it is unspecified. So I decided that unspecified is the actually the better way. (Maybe use DateTimeOffset?)

palhal commented 2 years ago

Thanks for the suggestion. I actually use both types. My model looks like this:

public DateTimeOffset Timestamp { get; set; }
public DateTime InsertedAtUtc { get; set; }

Timestamp is when an event actually occured. It's useful for our users to know the offset. (Yes, the API requires an offset to be included.) On the other hand, the insertion date is automatically set in the backend, and only UTC makes sense here. Maybe I'm overthinking, but I think that having the possibility to store an offset creates ambiguity.

springy76 commented 2 years ago

BTW: npgsql (postgres) provider just disallows (per default) any non-UTC DateTime, both for storage and query operations. And always return UTC, too - of course.

roji commented 2 years ago

@qsdfplkj

I didn't use this approach anymore because usually data comes in from an API and it is unspecified.

Carefully consider what exactly this means: are you really saying that you have no idea what time zone the timestamps are, which your API provides? If that's true, you can't really do anything with that data, since it could refer to arbitrary moments in time, etc. It's more common for the time zone to be simply implicit, e.g. for the timestamps to be in UTC or some very specific local timestamp - even if it's not specified. If that's the case, I highly recommend changing the DateTime's Kind to Utc (via DateTime.SpecifyKind), right where you're reading the timestamps, and not when writing them to the database (if you're deserializing JSON from the API, you can probably configure the JSON deserializer to do this for you as well).

Note that DateTimeOffset also implies that you know which time zone the timestamps are in.

qsdfplkj commented 2 years ago

I guess I've to revisit this but timestamp is always Utc. But what is a local times on a server in azure anyway?

roji commented 2 years ago

@qsdfplkj if the timestamp is UTC, then it should be represented in .NET as a DateTime with Kind=Utc (or possibly as DateTimeOffset with Offset=0). Do this as early as possible (i.e. as soon as you read the value from an external source), and persist it to PostgreSQL as a timestamp with time zone, things way everything is aligned across the board and there's no risk of accidental unexpected time zone conversions.

As to how Azure servers are configured, I have no idea - that likely depends how you set things up. But the server time zone should not matter: your application should be be using e.g. DateTime.UtcNow rather than DateTime.Now to get current timestamps in UTC, and so be oblivious of the server timestamp. The exception to using UTC should be when you really do care about the time zone (e.g. since some event needs to happen at some point in time in some user's time zone), in which case you need to store the time zone (not offset!) in your database (e.g. see this blog post).

ericbl commented 10 months ago

after reading all this discussion and the related npgsql doc, I am still a bit confused about what I should do in our very classic use case:

add-migration Initial properly create the DateTime (type: "timestamp with time zone" ) and the seed method with DateTimeKind.Utc

dotnet swagger generate the openapi.json with properties with "format": "date-time"

JsonSerializer also defined UTC per default AKAIK .

Everything seem to work fine in our first tests.

So apparently, there is NOTHING else to add, no need for DateTimeOffset, special converter and so one if UTC is the default all the way. However, the whole documentation is not very clear there...

roji commented 10 months ago

@ericbl the details vary somewhat across databases. Generally speaking, there's no need at all to switch to DateTimeOffset for using UTC timestamps; a DateTime with Kind=Utc will do just as well. However, since e.g. SQL Server doesn't store the Kind, DateTime instances read from it will have Kind=Unspecified; this could be a reason to use DateTimeOffset instead, since in that case an offset of 0 can be stored in the database. Or you can use an EF value converter to ensure that DateTimes read from the database have Kind=Utc. In PostgreSQL that particular problem doesn't exist.

Beyond that, if you have a specific question or difficulty, please let us know and we'll try to help.

crozone commented 6 months ago

So being able to round-trip DateTime's reliably basically boils down to:

public class UtcDateTimeConverter : ValueConverter<DateTime, DateTime>
{
    public UtcDateTimeConverter()
        : base(
            static datetime => datetime.ToUniversalTime(),
            static datetime => DateTime.SpecifyKind(datetime, DateTimeKind.Utc)
            )
    {
    }
}

And then manually applying this to every DateTime property everywhere.

EF really needs this as a built-in feature, with the ability to specify this behaviour as a global convention. The ability to actually store a zone offset is often unnecessary, rather we just want the DateTime round-tripped reliably.

Even if EF picks up the ability to set a global value converter without the need for all that reflection over entity properties, I think UTC DateTime conversion should still be a built-in feature, since it's a super common pitfall and all this boilerplate should ultimately be unnecessary.

roji commented 6 months ago

So being able to round-trip DateTime's reliably basically boils down to:

Note that this isn't what "round-tripping" means - your converter makes it so that all DateTimes are returned with Kind=Utc; in other words, the Kind property of the DateTime is lost.

I think UTC DateTime conversion should still be a built-in feature, since it's a super common pitfall and all this boilerplate should ultimately be unnecessary.

Not all users are actually interested in saving UTC timestamps in the database; many scenarios work only with local timestamps of a specific (implicit) timezone, and for those, Unspecified can be a good option.

We would in any case not change the default way that DateTime is written and read, as that would be a big breaking change; in addition, Unspecified DateTimes are returned by the database driver itself (SqlClient), and it's not EF's role to override that decision.

Given that we wouldn't change the default, including a type converter such as what you propose above still wouldn't remove the need to actually configure that converter; because of that, and because the actual converter code is very short/trivial, that seems to be quite low value...

ajcvickers commented 6 months ago

Agree with everything @roji said, but if you're interested, the available built-in conversions, are documented.

springy76 commented 6 months ago

@crozone just a side note on using ToUniversalTime() against DateTimeKind.Unspecified

var unspecified = new DateTime(2024, 05, 02, 12, 0, 0);
Console.WriteLine(unspecified.ToLocalTime());
Console.WriteLine(unspecified.ToUniversalTime());

ToLocalTime() handles DateTimes of unspecified kind as they would be of universal time. ToUniversalTime() handles DateTimes of unspecified kind as they would be of local time.

I'm quite happy with npgsqls decision to just throw on any non-utc value.