npgsql / efcore.pg

Entity Framework Core provider for PostgreSQL
PostgreSQL License
1.53k stars 224 forks source link

[Npgsql.EntityFrameworkCore.PostgreSQL.NodaTime] saving CLR DateTime field throws exception #568

Closed maciejjarzynski closed 6 years ago

maciejjarzynski commented 6 years ago

Hello, it seems that using of Npgsql.EntityFrameworkCore.PostgreSQL.NodaTime somehow blocks/corrupts working with CLR DateTime/DateTimeOffset types of properties. When trying to save DateTime field, an exception is being thrown: System.InvalidCastException : Can't write CLR type System.DateTime with handler type TimestampHandler

Steps to reproduce

.csproj references:

<PackageReference Include="Npgsql" Version="4.0.2" />
<PackageReference Include="Npgsql.EntityFrameworkCore.PostgreSQL" Version="2.1.1.1" />
<PackageReference Include="Npgsql.EntityFrameworkCore.PostgreSQL.NodaTime" Version="2.1.1" />
<PackageReference Include="Microsoft.EntityFrameworkCore" Version="2.1.1" />

Code:

public class NpgsqlNodaTimeTests
{
    public const string ConnectionString = "connection-string";

    [Fact]
    public async Task SavingDateTime_ShouldNotThrow()
    {
        var context = new BlogContext();

        context.Database.EnsureDeleted();
        context.Database.EnsureCreated();

        var blog = new Blog { Id = 1, Created = DateTime.Now };

        context.Blogs.Add(blog);

        await context.SaveChangesAsync();
    }
}

public class Blog
{
    public int Id { get; set; }
    public DateTime Created { get; set; }
}

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder builder)
        => builder.UseNpgsql(NpgsqlNodaTimeTests.ConnectionString, o => o.UseNodaTime());

    protected override void OnModelCreating(ModelBuilder builder)
        => builder.Entity<Blog>().HasKey(p => p.Id);
}

Exception

Microsoft.EntityFrameworkCore.DbUpdateException : An error occurred while updating the entries. See the inner exception for details.
---- System.InvalidCastException : Can't write CLR type System.DateTime with handler type TimestampHandler
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(DbContext _, ValueTuple`2 parameters, CancellationToken cancellationToken)
   at Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal.NpgsqlExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken) in C:\projects\EFCore.PG\src\EFCore.PG\Storage\Internal\NpgsqlExecutionStrategy.cs:line 72
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(IReadOnlyList`1 entriesToSave, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
   at NpgsqlNodaTimeTests.SavingDateTime_ShouldNotThrow() in -CUT--
--- End of stack trace from previous location where exception was thrown ---
----- Inner Stack Trace -----
   at lambda_method(Closure , NpgsqlTypeHandler , Object , NpgsqlLengthCache& , NpgsqlParameter )
   at Npgsql.TypeHandling.NpgsqlSimpleTypeHandler`1.ValidateObjectAndGetLength(Object value, NpgsqlLengthCache& lengthCache, NpgsqlParameter parameter) in C:\projects\npgsql\src\Npgsql\TypeHandling\NpgsqlSimpleTypeHandler.cs:line 236
   at Npgsql.NpgsqlParameter.ValidateAndGetLength() in C:\projects\npgsql\src\Npgsql\NpgsqlParameter.cs:line 553
   at Npgsql.NpgsqlCommand.ValidateParameters() in C:\projects\npgsql\src\Npgsql\NpgsqlCommand.cs:line 793
   at Npgsql.NpgsqlCommand.ExecuteDbDataReader(CommandBehavior behavior, Boolean async, CancellationToken cancellationToken) in C:\projects\npgsql\src\Npgsql\NpgsqlCommand.cs:line 1141
   at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.ExecuteAsync(IRelationalConnection connection, DbCommandMethod executeMethod, IReadOnlyDictionary`2 parameterValues, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)
roji commented 6 years ago

This is the expected behavior, at least for now.

At the Npgsql ADO level, a parameter type is typically specified by setting its NpgsqlDbType property: NpgsqlDbType.Timestamp indicates the PostgreSQL type timestamp without time zone. Internally, a type handler registers itself for handling specific values of NpgsqlDbType - it's a component which knows to take a .NET type and write it as the PostgreSQL type corresponding to the NpgsqlDbType.

Long story short, when you set up the ADO NodaTime plugin, it "hijacks" NpgsqlDbType.Timestamp and routes it to the NodaTime type handler, instead of the built-in DateTime type handler. The NodaTime type hander doesn't know how to write DateTime, so you get the above exception. EF Core, at its level, always sets NpgsqlDbType on parameters to unambiguously determine which PostgreSQL type it wants to send.

This unfortunately means that you can't mix both regular DateTime and NodaTime types in the same application.

Can you give some more context on your scenario, and why you need both types?

KrzysztofBranicki commented 6 years ago

@roji The scenario for using both is something like this: Let's assume we have a fairly big system. In this system most of the new code is using NodaTime but there is some amount of older code that is using built-in .NET types. Let's assume different areas of this older code are owned by different people. We would like to use Npgsql.EntityFrameworkCore.PostgreSQL.NodaTime to make adoption of NodaTime in the system event simpler and gradually step by step refactor places that still use .NET types. Unfortunately having this "all or nothing" approach forces us to have this "big bang" refactor where we need to remove all legacy DateTime usages at once before we can take advantage of all the goodness this library provides (which is not desired way to go).

That being said I think Npgsql.EntityFrameworkCore.PostgreSQL.NodaTime is big step forward in terms of NodaTime support in EF and great option especially when you are starting with greenfield project (in that case you could even argue that it is a good thing that we get an exception because no one would be able to use DateTime even if someone missed it in code review :)).

austindrenski commented 6 years ago

@KrzysztofBranicki Appreciate you sharing the additional info. It does make sense that you want to avoid the "big bang" refactor.

Have you tried rigging up a value converter (DateTime <=> NodaTime) as a shim during your transition?

roji commented 6 years ago

Have you tried rigging up a value converter (DateTime <=> NodaTime) as a shim during your transition?

This is a great suggestion, but unless I'm mistaken operations won't be translated to SQL with this method.

austindrenski commented 6 years ago

This is a great suggestion, but unless I'm mistaken operations won't be translated to SQL with this method.

Hmm... I wasn't thinking about this, but it might be worth investigating.

It doesn't actually seem out of the question though, since the plugin model just prepends the list of translators. So the default translators for DateTime should still be available, and as long as the value converter is the last thing to run before writing...maybe this would work?

I'll take a look and report back.

KrzysztofBranicki commented 6 years ago

Have you tried rigging up a value converter (DateTime <=> NodaTime) as a shim during your transition?

@austindrenski thanks for the suggestions we will definitely explore this as one of the options. Although we will be probably rescheduling our Noda refactor since it isn't just one extra configuration method and we have several other topics prioritized higher.

austindrenski commented 6 years ago

I'm going to close this for now, since this is currently the expected behavior. As @roji suspected, the value converter shim works, but operations are not translated.

Feel free to post back if you have any other questions, or to share how your NodaTime transition goes, as I imagine others may face similar challenges in the future.

krisztiankocsis commented 5 years ago

I have the same issue. We use IdentityFramework with approx. 20 additional entities. Unfortunately, it has must-have properties on User that are DateTimeOffset types. On the other hand, we use NodaTime types due to their advantages in our own entities. Since we derive our context from IdentityDbContext to share the database. It is not the best to have separate DbContexts just to separate identity and other data - not to mention if you have foreign keys.

I normally expected that Noda plugins just "adds" support for these types, not replaces support for DateTime. In my opinion, this should be supported in parallel.

Value converters are not options for us due to missing SQL translation in the current EF Core version (2.2).

What are your opinions?

roji commented 5 years ago

@krisztiankocsis this would be covered by https://github.com/npgsql/npgsql/issues/2439 (where you've also posted).

mbcrawfo commented 5 years ago

@roji is there a recommended workaround for this? I am also trying to use Identity with NodaTime and running into this issue on the types used by identity. The issue you were linking to in your last comment seems to be gone.

roji commented 5 years ago

@mbcrawfo unfortunately not - for now you have to either use the NodaTime types or the BCL types, but you can't use both.

thomasjedstrom commented 4 years ago

@roji Hey quick clarification on this, this means that the ValueConverter workaround isn't a viable option either? I'm using LocalTime/LocalDate and after initial hiccups, those seem to be converting ok, but Duration is throwing the invalid cast to TimeSpan and it seems like this gh issue explains why.

    public sealed class TimeEntry
    {
        public int Id { get; set; }
        public LocalDate WorkDate { get; set; }
        public LocalTime TimeIn { get; set; }
        public LocalTime TimeOut { get; set; }
        public Duration HoursWorked { get; set; }    // for this I needed the ValueConverter
    }

Context

           var durationConverter = 
                new ValueConverter<Duration, TimeSpan>(v =>  
                    v.ToTimeSpan(), 
                    v => Duration.FromTimeSpan(v));

            modelBuilder.Entity<TimeEntries.TimeEntry>()
                .Property(e => e.HoursWorked)
                    .HasConversion(durationConverter);

image

So in that case, you really can only use Noda with pg if you intend to stick with the types supported in this library so far, right?

roji commented 4 years ago

First, https://github.com/npgsql/npgsql/pull/3124 was recently merged, which means it will soon be fine to use both NodaTime and BCL date/time types on the same connection etc. (this should happen in the upcoming 5.0 release).

As of today, you can't natively use NodaTime types - including Duration - and BCL date/time types together; by natively I mean "without a value converter". If you want to use a value converter and convert from NodaTime to BCL types yourself, it will work (and you don't need to install the NodaTime plugin), but query translation won't work - you can't access properties on value-converted values inside a LINQ query.

Hope that clarifies things.